✨ Add conditions patch utils #3102
Conversation
k8s-ci-robot
added
size/L
util/conditions/patch.go
Outdated
| if sourceCondition := get(source, conditionPatch.Target.Type); sourceCondition != nil { | ||
| // if source and target agree on the change, then it is a conflict | ||
| if !hasSameState(sourceCondition, conditionPatch.Target) { | ||
| return nil, errors.Errorf("error patching conditions: The condition %q on was modified by a different process and this caused a merge conflict", conditionPatch.Target.Type) |
There was a problem hiding this comment.
I'm wondering if we should consider LastTransitionTime for the condition we are attempting to patch and allow updating the condition if our LastTransitionTime is newer than the LastTransitionTime from the source conditions?
There was a problem hiding this comment.
I struggled a little bit around the same problem and then I chose to not block for a small difference on LastTransitionTime while doing concurrent changes. Instead if there are changes in any other condition property, we are blocking.
Also, I opted for preserving the first LastTransitionTime stored in etcd, so we avoid flickering of conditions value for a matter of ms.
util/conditions/patch.go
Outdated
| // the target state corresponds. If not this is a conflict | ||
| if !reflect.DeepEqual(sourceCondition, conditionPatch.Base) { | ||
| if !hasSameState(sourceCondition, conditionPatch.Target) { | ||
| return nil, errors.Errorf("error patching conditions: The condition %q on was modified by a different process and this caused a merge conflict", conditionPatch.Target.Type) |
There was a problem hiding this comment.
Similar to above, if we have a newer LastTransitionTime than the associated condition in the source conditions, should we still allow updating the condition?
There was a problem hiding this comment.
As above, I don't think a small difference on LastTransitionTime while doing concurrent changes requires raising an error.
fabriziopandini
force-pushed
the
conditions-patch-utils
branch
from
e0ebd69 to
06f6f80
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/milestone v0.3.7 |
What this PR does / why we need it:
This PR implements a set of utils for handling patching of conditions, mostly designed for supporting the handling of concurrent changes to the same list of conditions
/assign @vincepri
/cc @benmoss
/cc @detiber