Convey failed maintenance by requestor #12
Conversation
| ConditionTypeReady string = "Ready" | ||
| // ConditionTypeFailed is the Failed Condition type for NodeMaintenance. | ||
| // It is used to convey failure during node maintenance operation by the requestor. | ||
| ConditionTypeFailed string = "Failed" |
There was a problem hiding this comment.
we could use a more specific condition like "MaintenanceFailed". open to suggestions.
ATM chose the minimalistic approach.
later on we can consider adding the "Degraded" condition type to convey possible errors during Maintenance prep by the operator.
There was a problem hiding this comment.
the name is fine, let's discuss the reason why we need a separate condition
There was a problem hiding this comment.
discussed offline.
it is desirable to have a separate condition so we dont have a mix of entities updating the same condition.
its is undesirable that one condition remains ready: true and the other Failed:true having two contradicting conditions at the same time.
thus we will
- keep the added condition which reflects the maintenance outcome by the requestor
- introduce a new condition reason to Ready condition : "RequestorFailedMaintenance". Ready condition will be false if this reason is set.
- controller will set above reason to Ready condition if "Failed" condition is detected.
- object will not be garbage collected if "RequestorFailedMaintenance" is set as Ready condition reason
Pull Request Test Coverage Report for Build 10580978064Details
💛 - Coveralls |
adrianchiris
force-pushed
the
add-fatal-condition
branch
from
71e29fc to
3b66cef
Compare
adrianchiris
changed the title
|
@ykulazhenkov PTAL. we now have:
the presence of 1. with state "True" will update "Ready" condition to Reason "RequestorFailed" if previous reason was "Ready" |
adrianchiris
force-pushed
the
add-fatal-condition
branch
from
3b66cef to
b8b2549
Compare
| newRCR := k8sutils.GetReadyConditionReason(newO) | ||
| condChanged := false | ||
|
|
||
| oldConds := make(map[string]metav1.Condition) |
There was a problem hiding this comment.
nit: why can't we sort the condition slice and then simply compare with reflect.DeepEqual?
There was a problem hiding this comment.
great idea :) will do it ofc.
There was a problem hiding this comment.
@ykulazhenkov PTAL, LMK if thats what you had in mind.
|
@adrianchiris the PR looks good. I added one nit comment that you can ignore |
adrianchiris
force-pushed
the
add-fatal-condition
branch
2 times, most recently
from
16b5693 to
e55534c
Compare
This commit adds support for requestors to convey failed node maintenance. - Add RequestorFailed condition type, to be set by requestors with reason MaintenanceFailed if maintenance operation failed - Add new condition reason to Ready condition : MaintenanceFailed. this reason is set by controller if Ready condition is with Ready reason and RequestorFailed condition is preset with value True. Failed condition type conveys failure in performing node maintenance by the requestor. Signed-off-by: adrianc <[email protected]>
adrianchiris
force-pushed
the
add-fatal-condition
branch
from
e55534c to
5318d07
Compare
This commit adds support for requestors to convey failed
node maintenance.
Add RequestorFailed condition type, to be set by requestors
with reason MaintenanceFailed if maintenance operation failed
Add new condition reason to Ready condition : MaintenanceFailed.
this reason is set by controller if Ready condition is with Ready
reason and RequestorFailed condition is preset with value True.
Failed condition type conveys failure in performing node
maintenance by the requestor.