Fix No modifications were requested issue during sdkUpdate for CacheSubnetGroup
#112
Conversation
ack-prow
bot
added
the
needs-ok-to-test
|
Hi @mustafasencer. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
ack-prow
bot
added
ok-to-test
| resp, err = rm.sdkapi.ModifyCacheSubnetGroupWithContext(ctx, input) | ||
| rm.metrics.RecordAPICall("UPDATE", "ModifyCacheSubnetGroup", err) | ||
| if err != nil { | ||
| if aerr, ok := err.(awserr.Error); ok { | ||
| switch aerr.Message() { | ||
| case "No modifications were requested.": | ||
| return desired, nil | ||
| } | ||
| } | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
IMO this is happening because the controller is trying to make an update request when it shouldn't. Idealy we should either fix the delta/readone first. And maybe eventually leverage OmitUnchangedField to generate the update function
There was a problem hiding this comment.
Based on your suggestion, I investigated a bit more and found that the post set output template I added (here) was fixing the primary issue where the controller fails with a Reconciler error (spec.subnetIDs: Required value) before even starting the Sync and thus not hitting sdkFind.
After the addition of a read many post set output template, now I see that the Sync starts with success and the template logic appends the ko.Spec.SubnetIDs slice with duplicate values which misleads the delta to which leads to updating the resource.
My quick solution: I am just checking if ko.Spec.SubnetIDs is populated or not before proceeding.
Another approach (perhaps): If there is another way to start Sync with success without a template addition, please advise, it could perhaps be a more sound approach 🤷
There was a problem hiding this comment.
I think we should not have duplicates at all - we need to focus on properly appending the subnets. Check my comment in the other PR https://github.com/aws-controllers-k8s/elasticache-controller/pull/111/files#r1133856530
@mustafasencer great job on testing and investigating this. I see that there were no elasticate releases 2 days ago but you still test the controllers - I'm curious to know how you do it :)
There was a problem hiding this comment.
@a-hilaly Thank for the prompt response! I addressed the comment in the new commit.
I am testing locally via the kind cluster where I build only the local image of the controller and test the adopted resource scenario.
mustafasencer
force-pushed
the
update-cache-subnet-group-custom-update
branch
from
8989b61
to
e38d709
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, mustafasencer 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:
Approvers can indicate their approval by writing |
Description of changes:
InvalidParameterValue: No modifications were requested.duringsdkUpdatejust after an adoption of an existing resource. I introduced a custom update for theCacheSubnetGroupresource in order to swallow the error duringsdkUpdate. Waiting your comments/reviews in case there is a better approach to handle this issue.For more context: #111 (review)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.