Eviction: Only reconcile on spec changes #172
Conversation
|
Draft, as it is untested. |
| WithValues("reason", statusCondition.Reason). | ||
| WithValues("msg", statusCondition.Message). | ||
| Info("unknown status condition") | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
hrm, instead returning inside the default branch, I would move it out again and remove the line 116.
|
|
||
| if expectHypervisor { | ||
| // Abort eviction | ||
| // Retry eviction |
There was a problem hiding this comment.
It says retry, but the next reconciliation will just exit.
Also, line 204/206, where the eviction is also aborted, doesn't bubble up the error - thus introducing inconsistency. I don't see a reason to have further reconciliation via error when we don't need a backoff.
| log := logger.FromContext(ctx) | ||
|
|
||
| hypervisor, err := openstack.GetHypervisorByName(ctx, r.computeClient, eviction.Spec.Hypervisor, false) | ||
|
|
There was a problem hiding this comment.
just visually, the previous line is related to the error check which makes sense to put together. But it's not super important to me.
| err := r.Patch(ctx, eviction, client.MergeFromWithOptions(evictionBase, client.MergeFromWithOptimisticLock{})) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I don't think this will work. We can either update the status subresource, or the metadata (finalizer), skipping the return here will almost always cause an error.
There was a problem hiding this comment.
True, I fear the rewrite will need to be more extensive then.
fwiesel
force-pushed
the
GenerationChangedPredicateEviction
branch
from
105b0a2 to
6097c6e
Compare
fwiesel
force-pushed
the
GenerationChangedPredicateEviction
branch
from
6097c6e to
3d62209
Compare
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
Closing it, as we will have to revisit this one later anyway with more wide-ranging changes. |
No description provided.