Simplify instance update code #1378
Conversation
Summary: we save one `Update` call in the `instance_controller.go::updateInstance` method by removing the finalizer *before* updating the instance. To avoid misleading log messages, the subsequent updating of the status is handling the fact that the instance might be removed. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
| // update instance spec and metadata. this will not update Instance.Status field | ||
| // The order of both updates below is important: *first* the instance Spec and Metadata and *then* the Status. | ||
| // If Status is update first, a new reconcile request will be scheduled with *WRONG* instance snapshot (which is | ||
| // saved in the annotations). This request will then have wrong "previous state". |
There was a problem hiding this comment.
👏 thanks for the docs and order procedures below!
| instanceStatus := instance.Status.DeepCopy() | ||
|
|
||
| if tryRemoveFinalizer(instance) { | ||
| log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name) |
There was a problem hiding this comment.
why is this not in the tryRemoveFinalizer? seems if this is valuable and used other places that we would want to know for all cases... what is logged is provided to the function.
There was a problem hiding this comment.
Good point, it's now there. That also allows us to distinguish the reasons why the finalizer was removed.
There was a problem hiding this comment.
Actually, #1352 moves this method (and it's adding-counterpart) into the helpers because they are not used by the webhook too. Now, log.Printf("InstanceController:... would be a lie. I think it is important to know who removed the finalizer so I kept it in the calling method. We could provide some logging prefix, of course, but that seems like overkill.
There was a problem hiding this comment.
On the other hand, I like the distinction between cleanup plan is finished and there is no cleanup plan. Ok, let's leave it like this.
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Summary: we save one `Update` call in the `instance_controller.go::updateInstance` method by removing the finalizer *before* updating the instance. To avoid misleading log messages, the subsequent updating of the status is handling the fact that the instance might be removed. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com> Co-authored-by: Andreas Neumann <aneumann@mesosphere.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>
Summary:
we save one
Updatecall in theinstance_controller.go::updateInstancemethod by removing the finalizer before updating the instance. To avoid misleading log messages, the subsequent updating of the status is handling the fact that the instance might be removed.Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com