Add NodeNameChangeExpected condition type #8
Conversation
pkg/conditions/conditions.go
Outdated
| // ProcessingType is the condition type used to signal the remediation has started and it is in progress, or has finished | ||
| ProcessingType = "Processing" | ||
| // SucceededType is the condition type used to signal whether the remediation was successful or not | ||
| SucceededType = "Succeeded" | ||
| // NodeNameChangeExpectedType is the condition type used to signal that the remediated node will have a new name | ||
| NodeNameChangeExpectedType = "NodeNameChangeExpected" |
There was a problem hiding this comment.
I'm assuming we are not tracking the new node, but just deleting the remediation when this condition is detected.
If this is indeed the case, maybe use a more generic condition, so it might be used in future operators as well ?
Something along the lines "Remove Remediation Regardless of healthy condition" (I'm leaving it to you to find an acceptable phrasing 😅 )
There was a problem hiding this comment.
Not this condition only must be true, also the succeeded condition, and the node must be deleted. So it's not about "ignore node conditions", but "you won't find the new node to check conditions at all".
There was a problem hiding this comment.
Not this condition only must be true, also the succeeded condition, and the node must be deleted
Would it make sense for MHC , to place this condition after success condition and node deletion are verified so in turn, for NHC it would be sufficient to only check this condition ? (that way we can have this condition used in other operators as well)
There was a problem hiding this comment.
There was a problem hiding this comment.
The condition message can have even more context, e.g. "Machine deleted on cloud provider, unhealthy node will be replaced by a node with a new name"
There was a problem hiding this comment.
I think it's a good trade-off 👍
I'm thinking on the condition message for a way to include the fact that "we cannot track the new node since it will have a new name", but I haven't come up with a good phrase yet
There was a problem hiding this comment.
oh, I missed a reply
Would it make sense for MHC , to place this condition after success condition and node deletion are verified so in turn, for NHC it would be sufficient to only check this condition ?
hm, I would prefer to keep the scope of each condition small, and let consumers of the conditions decide what to make out of them. Letting the PermanentNodeDeletionExpected condition implicitly also mark remediation as succeeded doesn't make sense to me.
There was a problem hiding this comment.
- Type is updated to
PermanentNodeDeletionExpected - I didn't add the reason, because that's remediator specific, and also NHC does not need it.
|
/lgtm |
|
/lgtm |
ECOPROJECT-1465