Comparing tags using common acktags.Tag structs
#1658
Labels
area/runtime
Issues or PRs as related to controller runtime, common reconciliation logic, etc
kind/enhancement
Categorizes issue or PR as related to existing feature enhancements.
service/all
Indicates issues or PRs related to all the service controllers.
Comments
RedbackThomson
added
the
needs-priority
ack-prow bot
pushed a commit
to aws-controllers-k8s/memorydb-controller
that referenced
this issue
Feb 6, 2023
…ion errors (#51) Issue #, if available: MemoryDB Controller has some flaky tests that randomly show assertion error. Root causes: 1. Snapshot test files didn't clean resources which were created from previous failed tests, so eventually test account reached node limit. 2. Delta compare considers order matters, so it considers two tag arrays are different even though they have same elements with different orders. Function of updating tags didn't include this corner case, and it caused infinite update. 3. When creating resources, some resources take a while to finish creation. Function of getting tags got error because it couldn't find resource, and this error covered previous ResourceNotFound error. Hence, reconciler didn't call requeue, because it didn't recognize the new error. 4. Cluster update validation test file has a step to update and validate Snapshot Window (daily time range to auto snapshot). If this step is executing within the time range of new Snapshot Window, update would succeed (ResourceSynced returns true) because two resources match, but cluster starts snapshotting after it's Snapshot Window is updated to the current time. Hence, for next step to update other fields of cluster, new fields couldn't be updated on time, because the status of cluster is snapshotting and cluster cannot be updated. Description of changes: 1. Fix updateTags function and it covers situation of equal tag arrays. (It still wastes time to call sdkUpdate over and over. It is better to modify delta comparison of tags in code-generator. New PR is created (aws-controllers-k8s/community#1658). 2. Change updateTags functions from parameter group and subnet group to match the updateTags functions in other resources. 3. Add a status check for getTags. It is only called when status of resource (cluster, snapshot, acl, user) is active. 4. Increase wait time for the next update step after Snapshot Window update. The new wait time is enough to cover snapshotting process. 5. Use same method to create cluster for snapshot for snapshot_validate_tags.yaml test file. 6. Correct user names and delete unused resources in test files. 7. Modify some test IDs, step IDs, and descriptions which are not appropriate that makes debugging really hard. 8. Fix import order of hook files. 9. Modify equalStrings function. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
kyriechen96
added a commit
to kyriechen96/memorydb-controller
that referenced
this issue
Feb 6, 2023
…ion errors (aws-controllers-k8s#51) Issue #, if available: MemoryDB Controller has some flaky tests that randomly show assertion error. Root causes: 1. Snapshot test files didn't clean resources which were created from previous failed tests, so eventually test account reached node limit. 2. Delta compare considers order matters, so it considers two tag arrays are different even though they have same elements with different orders. Function of updating tags didn't include this corner case, and it caused infinite update. 3. When creating resources, some resources take a while to finish creation. Function of getting tags got error because it couldn't find resource, and this error covered previous ResourceNotFound error. Hence, reconciler didn't call requeue, because it didn't recognize the new error. 4. Cluster update validation test file has a step to update and validate Snapshot Window (daily time range to auto snapshot). If this step is executing within the time range of new Snapshot Window, update would succeed (ResourceSynced returns true) because two resources match, but cluster starts snapshotting after it's Snapshot Window is updated to the current time. Hence, for next step to update other fields of cluster, new fields couldn't be updated on time, because the status of cluster is snapshotting and cluster cannot be updated. Description of changes: 1. Fix updateTags function and it covers situation of equal tag arrays. (It still wastes time to call sdkUpdate over and over. It is better to modify delta comparison of tags in code-generator. New PR is created (aws-controllers-k8s/community#1658). 2. Change updateTags functions from parameter group and subnet group to match the updateTags functions in other resources. 3. Add a status check for getTags. It is only called when status of resource (cluster, snapshot, acl, user) is active. 4. Increase wait time for the next update step after Snapshot Window update. The new wait time is enough to cover snapshotting process. 5. Use same method to create cluster for snapshot for snapshot_validate_tags.yaml test file. 6. Correct user names and delete unused resources in test files. 7. Modify some test IDs, step IDs, and descriptions which are not appropriate that makes debugging really hard. 8. Fix import order of hook files. 9. Modify equalStrings function. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
ack-prow bot
pushed a commit
to aws-controllers-k8s/code-generator
that referenced
this issue
Feb 9, 2023
Implements aws-controllers-k8s/community#1658 Description of changes: Instead of comparing tag fields using the standard `reflect.DeepEquals`, this PR updates the code-generator to convert the tags to the common `acktags.Tag` type and then compare those as maps. This ensures that changes in ordering do not result in false positive deltas. Controllers that rely on custom tag delta functions also typically use a function to determine which tags are being added and which are being removed. Using the same trick as this PR, a method can be added to the common runtime to deduce those - resulting in far less duplicated and verbose custom hook code. Those changes are not included in this PR, though. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
jljaco
added
area/runtime
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem?
Our current implementation for comparing tags is the same as any other field on the resource. That is, if the list of tags is a
map[string]stringwe use theMapStringStringPEqualmethod, or if the list of tags is a*[]Tagwe usereflect.DeepEqual.For lists of tags that are
*[]Tag,reflect.DeepEqualwill return a false positive if the order of the tags returned in theDescribe*call do not match the order in theSpec. This will trigger thesdkUpdatecall for no reason, adding extra cycles and possibly extraUpdate*calls unnecessarily.The common runtime is already able to detect a list of tags and convert them to a common
acktags.Tagtype, which is an analogue between any service-specific representation of tags. It would be straightforward to have a common delta code foracktags.Taglists and use that when comparing the tags within thedeltacode.Describe the solution you'd like
A new comparison operator for
[]acktags.Tagand a change to the delta code that detects when a field is a tag and uses the appropriate conversion and comparison to[]acktag.Tag.The text was updated successfully, but these errors were encountered: