Add Two Reasons for Updating and Stopping Remediation #76
Conversation
When either FAR CR name doesn't match a node name or Node Healthcheck timeout annotation has been set we update the CR three conditions to false
It is not necessary
|
/test 4.13-openshift-e2e |
RemediationStoppedByNHC -> RemediationInterruptedByNHC. NHC is signalling a timeout that interrupts the remediation and it does not stopping/terminating a remediation. FAR does that when it notice the annotation.
razo7
changed the title
| RemediationStartedConditionMessage = "FAR CR was found, the CR name matches one of the cluster nodes, and a finalizer was set" | ||
| FenceAgentSucceededConditionMessage = "FAR taint was added, fence agent command has been created and executed successfully" | ||
| RemediationFinishedConditionMessage = "The unhealthy node was fully remediated (it was tainted, fenced using FA and all the node resources have been deleted)" | ||
| RemediationInterruptedConditionMessage = "FAR CR name doesn't match a node name or Node Healthcheck timeout annotation has been set" |
There was a problem hiding this comment.
one reason for actual 2 reasons isn't that helpful for users, is it?
There was a problem hiding this comment.
Correct, I neglected the value of separating the messages.
…mediationInterruptedByNHC Clearer message for the users
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
| @@ -200,9 +200,9 @@ var _ = Describe("FAR Controller", func() { | |||
| testPodDeletion(testPodName, resourceDeletionWasTriggered) | |||
|
|
|||
| By("Not having any condition set") | |||
| processingConditionStatus = metav1.ConditionFalse | ||
| fenceAgentActionSucceededConditionStatus = metav1.ConditionFalse | ||
| succeededConditionStatus = metav1.ConditionFalse | ||
| if reason == v1alpha1.RemediationFinishedNodeNotFound { |
There was a problem hiding this comment.
IMHO a switch makes more sense here, which errors when a reason isn't handled. Prevents forgetting this part when adding another reason to the case statement above.
It helps us forgetting this part when adding another reason to the case statement above.
| @@ -166,7 +166,7 @@ var _ = Describe("FAR Controller", func() { | |||
| testVADeletion(vaName2, resourceDeletionWasTriggered) | |||
| testPodDeletion(testPodName, resourceDeletionWasTriggered) | |||
|
|
|||
| By("Having Succeed condition set to true") | |||
| By("Having Succeeded, FenceAgentActionSucceeded conditions set to true, and Processing set to false") | |||
There was a problem hiding this comment.
just a general note: I would avoid being too detailed in the these comments, and use something more generic like "Verifying correct conditions". The details are very readable in the actual test code, and in the test logs in case a test fails. That way you don't need to update the comment ever time the test changes 🙂
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7, slintes 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 |
|
/retest |
1 similar comment
|
/retest |
false:-
RemediationInterruptedByNHC- FAR CR name doesn't match a node name-
RemediationFinishedNodeNotFound- Node Healthcheck timeout annotation has been set