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

Avoid reconciliation loops with predicates #268

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Avoid reconciliation loops with predicates #268

merged 1 commit into from
Jan 11, 2021

Conversation

ccremer
Copy link
Contributor

@ccremer ccremer commented Jan 8, 2021

Summary

From the godoc:


GenerationChangedPredicate implements a default update predicate function on Generation change.
This predicate will skip update events that have no change in the object's metadata.generation field.
The metadata.generation field of an object is incremented by the API server when writes are made to the spec field of an object.
This allows a controller to ignore update events where the spec is unchanged, and only the metadata and/or status fields are changed.
For CustomResource objects the Generation is only incremented when the status subresource is enabled.
Caveats:

  • The assumption that the Generation is incremented only on writing to the spec does not hold for all APIs.
    E.g For Deployment objects the Generation is also incremented on writes to the metadata.annotations field.
    For object types other than CustomResources be sure to verify which fields will trigger a Generation increment when they are written to.
  • With this predicate, any update events with writes only to the status field will not be reconciled.
    So in the event that the status block is overwritten or wiped by someone else the controller will not self-correct to restore the correct status.

  • In short: This should avoid a reconciliation loop when we update the status by ourselves. but also means that if someone else manually updates the status field then no reconciliation will be triggered either. I don't a reason why a user would do that, but hey...
  • May also be relevant for K8up spams the same jobs to the queue #257

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking,
    as they show up in the changelog
  • Link this PR to related issues.

This should avoid a reconciliation loop when we update the status by ourselves.
@ccremer ccremer added the change Generic change that is neither a fix or feature label Jan 8, 2021
@ccremer ccremer added this to the v1.0.0-rc3 milestone Jan 8, 2021
@cimnine
Copy link
Contributor

cimnine commented Jan 11, 2021

May also be relevant for #257

#257 should be fixed with #266, it wasn't a problem with too many reconciliations but rather a problem of updating the status information. This PR would not have fixed that problem.

Still, I believe this PR will avoid unnecessary reconciliation loops which improves many factors of k8up (i.e. load and log output, possibly others)

@ccremer ccremer marked this pull request as ready for review January 11, 2021 15:03
@ccremer ccremer merged commit 15f3dfb into master Jan 11, 2021
@ccremer ccremer deleted the predicate branch January 11, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Generic change that is neither a fix or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants