-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Use predicate in finalizer controllers to only process update events. #7941
base: main
Are you sure you want to change the base?
Conversation
/kind changelog-not-required |
9261b6f
to
9307517
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7941 +/- ##
==========================================
- Coverage 58.79% 58.79% -0.01%
==========================================
Files 345 346 +1
Lines 28766 28917 +151
==========================================
+ Hits 16914 17002 +88
- Misses 10423 10483 +60
- Partials 1429 1432 +3 ☔ View full report in Codecov by Sentry. |
if !ok { | ||
return false | ||
} | ||
return backup.Status.Phase == velerov1api.BackupPhaseFinalizing || backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should return the opposite result here.
Restore reconciler should also be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Update returns true if the Update event should be processed
UpdateFunc func(event.UpdateEvent) bool
Don't we want events to be processed if CR was updated to finalizing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaovilai so there are 2 cases where we want to process backups in this controller:
- backups updated to Finalizing or FinalizingPartiallyFailed phase
- backups already in finalizing phase that returned err from prior reconcile (i.e. requeueing)
Does this predicate change affect requeue behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes, for the predicate, it looks like we want to return true for backups in these two phases.
if !ok { | ||
return false | ||
} | ||
return backup.Status.Phase == velerov1api.BackupPhaseFinalizing || backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaovilai so there are 2 cases where we want to process backups in this controller:
- backups updated to Finalizing or FinalizingPartiallyFailed phase
- backups already in finalizing phase that returned err from prior reconcile (i.e. requeueing)
Does this predicate change affect requeue behavior?
if !ok { | ||
return false | ||
} | ||
return backup.Status.Phase == velerov1api.BackupPhaseFinalizing || backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes, for the predicate, it looks like we want to return true for backups in these two phases.
Signed-off-by: Tiger Kaovilai <[email protected]>
9307517
to
e62d4a8
Compare
Signed-off-by: Tiger Kaovilai [email protected]
Thank you for contributing to Velero!
Please add a summary of your change
Does your change fix a particular issue?
Fixes #7868
Please indicate you've done the following:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
./kind changelog-not-required