provider-aws icon indicating copy to clipboard operation
provider-aws copied to clipboard

Published 20 hours ago • verified publisher icon crossplane-contrib

add change predication for all controllers

Open liubog2008 opened this issue 2 years ago • 1 comments

Description of your changes

Fixes #1572

I have:

  • [x] Read and followed Crossplane's contribution process.
  • [ ] Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

liubog2008 avatar Nov 24 '22 08:11 liubog2008

@muvaf any chance to check this change ? or is this something for the runtime ?

haarchri avatar Dec 05 '22 21:12 haarchri

@haarchri @muvaf any progressing?

liubog2008 avatar Dec 21 '22 01:12 liubog2008

@liubog2008 Please update this PR with more information around how this fixes the issue.

I tend to agree with @haarchri that it may make sense to try fix this in crossplane-runtime rather than updating every controller in every provider manually.

negz avatar Jan 03 '23 22:01 negz

@negz You could see this issue https://github.com/crossplane-contrib/provider-aws/issues/1572 for the context.

For func will enqueue all objects in UpdateEvent directly without After and re-trigger the Reconcile. However, sometimes the status will be updated every Reconcile. For example, aws request id will be updated every time if deletion failed and the backoff will not be worked.

reconcile -> update status -> handle update event -> enqueue directly -> reconcile again

I filter the UpdateEvent which doesn't update the obj spec, annotations and labels to avoid the unexpected requeue action.

reconcile -> update status -> handle update event -> ignore this updation -> wait until the backoff is finished -> enqueue -> reconcile again

liubog2008 avatar Jan 12 '23 02:01 liubog2008

I have this forked and built into v0.30.1

Here is my result (metric is in request per second to the aws api)

crossplaneRPS

This type of problem can cost a company THOUSANDS of dollars a month in AWS billing as Cloudtrail is very un-forgiving. Even with this fix i would argue that the back-off is not high enough, i still see too many api calls.

tehlers320 avatar Jan 12 '23 15:01 tehlers320

I'm not sure updating every controller in every provider is the way to go. We might want to add something to crossplane-runtime to help.

LGTM, my fix is not elegant. I think a new For function is more reasonable.

liubog2008 avatar Jan 13 '23 01:01 liubog2008

Closing in favor of https://github.com/crossplane-contrib/provider-aws/pull/1705

turkenh avatar Mar 21 '23 09:03 turkenh