fix: make service instance reconciler ignore status changes - #59
fix: make service instance reconciler ignore status changes#59Diaphteiros wants to merge 4 commits into
Conversation
Signed-off-by: Johannes Aubart <johannes.aubart@sap.com>
Signed-off-by: Johannes Aubart <johannes.aubart@sap.com>
|
@Diaphteiros added a new |
| func defaultControllerConfig() *controllerConfig { | ||
| return &controllerConfig{ | ||
| forPredicates: builder.WithPredicates( | ||
| predicate.And( | ||
| predicate.Or( | ||
| predicate.GenerationChangedPredicate{}, | ||
| controllerutil2.DeletionTimestampChangedPredicate{}, | ||
| predicate.LabelChangedPredicate{}, | ||
| predicate.AnnotationChangedPredicate{}, | ||
| ), | ||
| predicate.Not( | ||
| controllerutil2.HasAnnotationPredicate(apiconst.OperationAnnotation, apiconst.OperationAnnotationValueIgnore), | ||
| ), | ||
| ), | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
If a resource has the ignore annotation and will be deleted, the resource will be stuck in termination until the ignore annotation will be removed.
The question is, shall the ignore annotatation also prevent the deletion from happening? If not, the Not(HasAnnotation(ignore) needs to handle deletion timestamp events.
There was a problem hiding this comment.
Good point, I never thought of that. This is close to my 'default' predicate template which I use in most controllers I build.
However, I don't think that this will be a big issue in reality: The ignore annotation is meant to be a quick means for operators to temporarily prevent a controller from reconciling a resource. It is not expected to be on resources for a long time, and whoever set the annotation should be aware that this might cause issues.
Since the annotation's purpose is basically to mark the resource as 'don't reconcile this', I would argue that the resource being stuck in deletion because its deletion flow is not triggered is actually the expected (and a completely valid) result.
TL;DR: Didn't think of this, but also don't think we need to do anything here.
There was a problem hiding this comment.
To me it would be fine to remove the (And +) Not part but imo the main point is the helpful log entry that we essentially remove otherwise #59 (comment)
| predicate.AnnotationChangedPredicate{}, | ||
| ), | ||
| predicate.Not( | ||
| controllerutil2.HasAnnotationPredicate(apiconst.OperationAnnotation, apiconst.OperationAnnotationValueIgnore), |
There was a problem hiding this comment.
Events with ignore annotation present will now be filtered before reaching the reconcile handler.
This is ok, but any logs that would have been emitted previously are now silenced which could now be confusing when debugging a problem.
There was a problem hiding this comment.
I agree, but I see more the positive aspect of this: Most controllers log something when starting a reconciliation. Checking for the ignore annotation only later will result in some 'wrong' logs - 'wrong' in the sense that the controller logs a reconciliation, but that reconciliation is actually not happening (aborted at the first opportunity).
Using this predicate therefore prevents some confusing logs. Missing reconciliations, which should only happen if the predicates are unfit (should not happen with our defaults), can still be detected by the absence of the aforementioned 'Starting reconcile' log message.
3 participants
What this PR does / why we need it:
Service providers are usually the only controller modifying a service instance resource's status, which means that they don't need to react to status changes, as they caused them themselves.
Excluding status changes from triggering reconciliations reduces the chance for infinite reconciliation loops, which will otherwise happen if the service resource's status changes with every reconciliation.
Which issue(s) this PR fixes:
Fixed a problem with the service provider ODG.
Special notes for your reviewer:
Currently a draft because we want to add the option to overwrite the default predicates.Release note: