Skip to content

Fix delta comparison for auto failover, multi AZ, primary cluster ID #69

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 17, 2021
Merged

Fix delta comparison for auto failover, multi AZ, primary cluster ID #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion apis/v1alpha1/ack-generate-metadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ api_directory_checksum: 8e4808b17b3a814f0a624eee8d68a0b45ba5e2c4
api_version: v1alpha1
aws_sdk_go_version: v1.38.52
generator_config_info:
file_checksum: 28e3cd3119df8028a65eb0729a462b4354d27fa9
file_checksum: 23f444fb86eaa5c1acb4d4f51430e606e7ee554a
original_file_name: generator.yaml
last_modification:
reason: API generation
11 changes: 10 additions & 1 deletion apis/v1alpha1/generator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ resources:
from:
operation: ListAllowedNodeTypeModifications
path: ScaleDownModifications
AutomaticFailoverEnabled:
compare:
is_ignored: true
Events:
is_read_only: true
from:
Expand All @@ -62,6 +65,12 @@ resources:
path: ReplicationGroup.LogDeliveryConfigurations
compare: # removes the spec field from automatic delta comparison
is_ignored: true
MultiAZEnabled:
compare:
is_ignored: true
PrimaryClusterId: # note: "PrimaryClusterID" will not function properly
Copy link
Member

@a-hilaly a-hilaly Oct 26, 2021

Choose a reason for hiding this comment

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

Interesting, maybe we should open an Issue for this?

compare:
is_ignored: true
hooks:
sdk_read_many_post_set_output:
template_path: hooks/replication_group/sdk_read_many_post_set_output.go.tpl
Expand All @@ -72,7 +81,7 @@ resources:
sdk_update_post_build_request:
template_path: hooks/replication_group/sdk_update_post_build_request.go.tpl
delta_post_compare:
code: "filterDelta(delta, a, b)"
code: "modifyDelta(delta, a, b)"
sdk_file_end:
template_path: hooks/replication_group/sdk_file_end.go.tpl
sdk_file_end_set_output_post_populate:
Expand Down
11 changes: 10 additions & 1 deletion generator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ resources:
from:
operation: ListAllowedNodeTypeModifications
path: ScaleDownModifications
AutomaticFailoverEnabled:
compare:
is_ignored: true
Events:
is_read_only: true
from:
Expand All @@ -62,6 +65,12 @@ resources:
path: ReplicationGroup.LogDeliveryConfigurations
compare: # removes the spec field from automatic delta comparison
is_ignored: true
MultiAZEnabled:
compare:
is_ignored: true
PrimaryClusterId: # note: "PrimaryClusterID" will not function properly
compare:
is_ignored: true
hooks:
sdk_read_many_post_set_output:
template_path: hooks/replication_group/sdk_read_many_post_set_output.go.tpl
Expand All @@ -72,7 +81,7 @@ resources:
sdk_update_post_build_request:
template_path: hooks/replication_group/sdk_update_post_build_request.go.tpl
delta_post_compare:
code: "filterDelta(delta, a, b)"
code: "modifyDelta(delta, a, b)"
sdk_file_end:
template_path: hooks/replication_group/sdk_file_end.go.tpl
sdk_file_end_set_output_post_populate:
Expand Down
23 changes: 1 addition & 22 deletions pkg/resource/replication_group/delta.go
View file
Open in desktop

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

90 changes: 88 additions & 2 deletions pkg/resource/replication_group/delta_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ import (
"github.com/aws-controllers-k8s/elasticache-controller/pkg/common"
)

// filterDelta removes non-meaningful differences from the delta and adds additional differences if necessary
func filterDelta(
// modifyDelta removes non-meaningful differences from the delta and adds additional differences if necessary
func modifyDelta(
delta *ackcompare.Delta,
desired *resource,
latest *resource,
Expand Down Expand Up @@ -54,6 +54,19 @@ func filterDelta(
delta.Add("Spec.LogDeliveryConfigurations", desired.ko.Spec.LogDeliveryConfigurations,
unmarshalLastRequestedLDCs(desired))
}

if multiAZRequiresUpdate(desired, latest) {
delta.Add("Spec.MultiAZEnabled", desired.ko.Spec.MultiAZEnabled, latest.ko.Status.MultiAZ)
Copy link
Contributor

@jaypipes jaypipes Nov 9, 2021

Choose a reason for hiding this comment

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

Why isn't the Spec.MultiAZEnabled field being set to the return value of the ReadOne operation's Output shape's MultiAZ field?

Copy link
Contributor Author

@echen-98 echen-98 Nov 11, 2021

Choose a reason for hiding this comment

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

They are different types; see response below for more detail

}

if autoFailoverRequiresUpdate(desired, latest) {
delta.Add("Spec.AutomaticFailoverEnabled", desired.ko.Spec.AutomaticFailoverEnabled,
latest.ko.Status.AutomaticFailover)
}

if updateRequired, current := primaryClusterIDRequiresUpdate(desired, latest); updateRequired {
delta.Add("Spec.PrimaryClusterID", desired.ko.Spec.PrimaryClusterID, *current)
}
Comment on lines +58 to +69
Copy link
Member

@a-hilaly a-hilaly Oct 21, 2021

Choose a reason for hiding this comment

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

Since filterDelta does not only remove deltas anymore, maybe put this logic in a different function? Or rename it to something more generalistic?

}

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

return lastRequestedConfigs
}

// multiAZRequiresUpdate returns true if the latest multi AZ status does not yet match the
// desired state, and false otherwise
func multiAZRequiresUpdate(desired *resource, latest *resource) bool {
// no preference for multi AZ specified; no update required
if desired.ko.Spec.MultiAZEnabled == nil {
return false
Copy link
Member

@a-hilaly a-hilaly Oct 21, 2021

Choose a reason for hiding this comment

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

If desired.ko.Spec.MultiAZEnabled is nil or &false, and latest.ko.Spec.MultiAZEnable is &true no updates are required? there are no API calls to make such update?

Copy link
Contributor Author

@echen-98 echen-98 Oct 21, 2021

Choose a reason for hiding this comment

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

If it is nil, then the user has no preference and the server will select the default value (can't remember if it's enabled or disabled off the top of my head).

One thing to note is that latest.ko.Spec.MultiAZEnabled will always the same here as desired.ko.Spec.MultiAZEnabled because latest is copied from desired and MultiAZEnabled (bool) is not part of the DescribeReplicationGroups API response, so we get the status directly from MultiAZ (string/enum).

If desired.ko.Spec.MultiAZEnabled is &false and *latest.ko.Spec.MultiAZ is "enabled", then line 134 handles this case and there will be an API call to disable multi AZ. The test test_rg_fault_tolerance covers this scenario.

}

// API should return a non-nil value, but if it doesn't then attempt to update
if latest.ko.Status.MultiAZ == nil {
return true
}

// true maps to "enabled"; false maps to "disabled"
// this accounts for values such as "enabling" and "disabling"
if *desired.ko.Spec.MultiAZEnabled {
return *latest.ko.Status.MultiAZ != string(svcapitypes.MultiAZStatus_enabled)
} else {
return *latest.ko.Status.MultiAZ != string(svcapitypes.MultiAZStatus_disabled)
}
Comment on lines +130 to +136
Copy link
Contributor

@jaypipes jaypipes Nov 9, 2021

Choose a reason for hiding this comment

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

Two things:

  1. There should not be a Status.MultiAZ field. There should only be a single field: Spec.MultiAZEnabled

  2. This kind of custom code should go in your updateSpecFields function in custom_set_output.go and look something like this:

	// MultiAZEnabled is a boolean in the Spec field but a string enum in the
	// output shape field. Here, we reconcile that difference...
	if (respRG.MultiAZ == nil) {
		// Or set this to a default value if nil isn't default?
		resource.ko.Spec.MultiAZEnabled = nil
	} else {
		resource.ko.Spec.MultiAZEnabled = (*respRG.MultiAZ == string(svcapitypes.MultiAZStatus_enabled))
	}

If you do the above, you don't have to worry about delta comparisons at all.

Copy link
Contributor Author

@echen-98 echen-98 Nov 11, 2021

Choose a reason for hiding this comment

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

There should not be a Status.MultiAZ field. There should only be a single field: Spec.MultiAZEnabled

Agreed in principle, but the existence of a separate MultiAZ field is an ElastiCache API quirk rather than a conscious decision on our part.

This kind of custom code should go in your updateSpecFields function in custom_set_output.go and look something like this:

I'm glad you brought this up because it's helping me externalize why we're moving away from updateSpecFields in many cases.

Initially, I wanted to use updateSpecFields to update every single spec field during ReadOne, so that the generated delta code could handle everything and there would be no need for manual comparison between desired.Spec and latest.Status.

However, Spec fields often don't allow us to store all the information we're getting from the API response. For example, Spec.AutomaticFailoverEnabled is a bool, whose associated field Status.AutomaticFailover is a string/enum which could be any of "enabled", "disabled", "enabling", or "disabling". We would have to map each of these four states back to a bool (square peg, round hole sort of thing), when it would make more sense to just make a decision based on the full version of the information (Status) since we have it anyway.

Granted, the MultiAZ case is a little different since at the moment, the only two possible values are "enabled" and "disabled". However, it's entirely possible that in the future ElastiCache decides to have this field update asynchronously or make any other change which could introduce additional states/values for Status.MultiAZ. So the suggested approach could break given these changes, and even if no service-side changes occur, I would say it's still non-ideal to force a string/enum into a bool when we can just make a decision based on the full, unadulterated information from the API response.

As far as the role of updateSpecFields, I think it absolutely should exist, but the function name and intended purpose should change. The majority of this function is already calling the DescribeCacheClusters API and populating the associated fields in the replication group object, since the latest state for some Spec fields cannot be retrieved directly from the DescribeReplicationGroups API. I would say that this should be the primary purpose of this function in the future.

So to sum up, the function currently named updateSpecFields is an extension of SdkFind/ReadOne in that we are still retrieving the latest state of the replication group (but via a different API). There should be limited mapping back to Spec fields to avoid losing information; we only do it here because this state will be lost if we don't save it back to the latest object. On the other hand, modifyDelta should look at the most complete representation of the latest state of each Spec field available to it (which is often in the Status) and make a decision based on that. Therefore I think we should keep things as-is.

}

// autoFailoverRequiresUpdate returns true if the latest auto failover status does not yet match the
// desired state, and false otherwise
func autoFailoverRequiresUpdate(desired *resource, latest *resource) bool {
// the logic is exactly analogous to multiAZRequiresUpdate above
if desired.ko.Spec.AutomaticFailoverEnabled == nil {
return false
}

if latest.ko.Status.AutomaticFailover == nil {
return true
}

if *desired.ko.Spec.AutomaticFailoverEnabled {
return *latest.ko.Status.AutomaticFailover != string(svcapitypes.AutomaticFailoverStatus_enabled)
} else {
return *latest.ko.Status.AutomaticFailover != string(svcapitypes.AutomaticFailoverStatus_disabled)
}
}

// primaryClusterIDRequiresUpdate retrieves the current primary cluster ID and determines whether
// an update is required. If no desired state is specified or there is an issue retrieving the
// latest state, return false, nil. Otherwise, return false or true depending on equality of
// the latest and desired states, and a non-nil pointer to the latest value
func primaryClusterIDRequiresUpdate(desired *resource, latest *resource) (bool, *string) {
Copy link
Member

@a-hilaly a-hilaly Oct 26, 2021

Choose a reason for hiding this comment

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

Tiny QQ: Any reason why are you using a *string and not just a string?

if desired.ko.Spec.PrimaryClusterID == nil {
return false, nil
}

// primary cluster ID applies to cluster mode disabled only; if API returns multiple
// or no node groups, or the provided node group is nil, there is nothing that can be done
if len(latest.ko.Status.NodeGroups) != 1 || latest.ko.Status.NodeGroups[0] == nil {
return false, nil
}

// attempt to find primary cluster in node group. If for some reason it is not present, we
// don't have a reliable latest state, so do nothing
ng := *latest.ko.Status.NodeGroups[0]
for _, member := range ng.NodeGroupMembers {
if member == nil {
continue
}

if member.CurrentRole != nil && *member.CurrentRole == "primary" && member.CacheClusterID != nil {
Copy link
Member

@a-hilaly a-hilaly Oct 21, 2021

Choose a reason for hiding this comment

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

QQ: you can only have one "primary" cluster in a node group?

Copy link
Contributor Author

@echen-98 echen-98 Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Correct, and primary "cluster" (it's just a node) ID can only be specified when there is one node group in the replication group.

val := *member.CacheClusterID
return val != *desired.ko.Spec.PrimaryClusterID, &val
}
}

return false, nil
}
5 changes: 2 additions & 3 deletions test/e2e/tests/test_replicationgroup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,6 @@ def test_rg_update_misc(self, rg_update_misc_input, rg_update_misc):
assert_misc_fields(reference, rg_update_misc_input['RG_ID'], pmw, description, srl, sw)

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

# assert AF and multi AZ
assert resource['status']['automaticFailover'] == "disabled"
assert resource['status']['multiAZ'] == "disabled"
assert resource['status']['automaticFailover'] == "enabled"
Copy link
Contributor Author

@echen-98 echen-98 Oct 21, 2021

Choose a reason for hiding this comment

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

these were just wrong assertions; if you check the file the last patch enables both of these fields

assert resource['status']['multiAZ'] == "enabled"

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