Skip to content
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.

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

feat: Do not set Status.AtProvider.LastUsedAt for IAM roles by default #2064

Conversation

max-melentyev
Copy link
Contributor

Because it can call frequent unnecessary reconciliations of dependent resources.

Description of your changes

LastUsedAt is updated frequently and triggers unnecessary reconciliations of dependent resources.

I presumed that this field is used rarely and reasonable default would be not to update it on every reconciliation, even though this will be a breaking change.

Please let me know if you think it's better to use IGNORE_LAST_USED_AT annotation instead.

I have:

  • 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

new test case

As it can call frequent unnecessary reconciliations of
dependent resources.

Signed-off-by: Max Melentyev <max.melentyev@reddit.com>
@MisterMX
Copy link
Collaborator

Can you give more information about the issue you are experiencing? Which reconcilations are triggered by an update of the roles status information?

@max-melentyev
Copy link
Contributor Author

We have a CRD and its controller manages roles used by the CRD. This controller watches all its managed resources including roles to make sure these resources exist and have required specs. This makes controller-runtime to run controller every time watched resource changes. Roles are used often and this makes CRD controller run constantly even if nothing else happens.

@MisterMX
Copy link
Collaborator

I'm not very confident with adding this exception for a very special use case. As long as there no other users having the same issue in their environment I would suggest to not merge this and propose to use a custom fork instead.

@MisterMX MisterMX closed this Jul 24, 2024
@max-melentyev max-melentyev deleted the iam-role-upstream branch July 24, 2024 18:10
@max-melentyev
Copy link
Contributor Author

max-melentyev commented Jul 24, 2024

@MisterMX What if I change it to the "default to the current behaviour" and make new behaviour optional?

@max-melentyev max-melentyev restored the iam-role-upstream branch July 24, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants