Making finalizer removal even more readable #1393
Conversation
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
| @@ -295,7 +292,7 @@ func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Ins | |||
| // if k8s GC was fast and managed to removed the instance (after the above Update removed the finalizer), we get an untyped | |||
| // "StorageError" telling us that the sub-resource couldn't be modified. We ignore the error (but log it just in case). | |||
| // historically we checked with a kerrors.IsNotFound() which failed based on the StorageError. Perhaps this is a k8s bug? | |||
| if isInstanceDeleted { | |||
| if instance.IsDeleting() && finalizerRemoved { | |||
There was a problem hiding this comment.
what if the instance does not have cleanup plan, thus does not have finalizer? The previous case still fallen in here, the new does not.
There was a problem hiding this comment.
Lol, you're right 😂 I feel like tryRemoveFinalizer needs an overhaul because we're trying to work around its quirks. I'm looking at it even closer and this else statement is actually not true!:
if planStatus := i.PlanStatus(v1beta1.CleanupPlanName); planStatus != nil {
...
}
else {
// We have a finalizer but no cleanup plan. This could be due to an updated instance.
// Let's remove the finalizer.
...
return true
}
We're in fact not checking the existence of a plan, but only the existence of the according PlanStatus. And it is initialized in the middle of controller reconciliation while this method might be called even before that (or even in the webhook)
There was a problem hiding this comment.
this feels like a lot of assumptions and hoop jumping... couldn't we just check the type of the err for the StorageError?
There was a problem hiding this comment.
Only with string compare, AFAIR. It's an error that's not wrapped as a nice typed error :-/
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Co-Authored-By: Ken Sipe <kensipe@gmail.com> Co-Authored-By: Andreas Neumann <aneumann@mesosphere.com> Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>
Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com
Co-Authored-By: Andreas Neumann aneumann@mesosphere.com
Co-Authored-By: Ken Sipe kensipe@gmail.com