Improve Verification of FA Success Response #70
Conversation
|
Skipping CI for Draft Pull Request. |
razo7
force-pushed
the
fix-flaky-log-test
branch
from
ced3103
to
f762d00
Compare
|
/test 4.13-openshift-e2e |
test/e2e/far_e2e_test.go
Outdated
| func checkFarLogs(logString string) { | ||
| // buildExpectedLogOutput returns a string with a node identifier and a success message for the reboot action | ||
| func buildExpectedLogOutput(nodeName, sucesssMessage string) string { | ||
| expectedString := "\"Node name\": \"" + nodeName + "\", \"Response\": \"" + sucesssMessage |
There was a problem hiding this comment.
is there a reason not to use fmt.Sprintf? I should be easier to read
razo7
force-pushed
the
fix-flaky-log-test
branch
from
6dcd118
to
5f406e9
Compare
|
/test 4.13-openshift-e2e |
When reboot is running only once and it is running on FAR node, then FAR pod will be recreated on a new node and since the FA command won't be executed again, then the log won't include any success message, so we won't verify the FAR success message for this scenario
checkFarLogs will use node name in order to verify a successful response message, since prior to this change checkFarLogs searched for the existance of success message which could have been from an earlier remediation
razo7
force-pushed
the
fix-flaky-log-test
branch
from
5f406e9
to
3d82d20
Compare
|
/test 4.13-openshift-e2e |
test/e2e/far_e2e_test.go
Outdated
| EventuallyWithOffset(1, func() string { | ||
| pod, err := utils.GetFenceAgentsRemediationPod(k8sClient) | ||
| if err != nil { | ||
| log.Error(err, "failed to get FAR pod. Might try again") | ||
| return "" | ||
| } | ||
| if pod.Spec.NodeName == farNodeName { |
There was a problem hiding this comment.
Nit: IIUC farNodeName is the node that being remidiated.
I think that the name is a bit confusing in that context (initially I thought farNodeName is the node that Far Pod is running on).
There was a problem hiding this comment.
Thanks, it was indeed confusing.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clobrano, razo7 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 |
|
I think it is safe to merge |
checkFarLogswas used to verify whether FAR pod had a success response from FA command.When we are running more than one remediation then
checkFarLogsfunction could have find the success message of the first CR since we have searched for the response without any CR identification (see old discussion). Now we are using the node name to verify that (since we won't remediate a node more than once).When the FAR pod resides in the unhealthy node, then it will be evicted and one FA command is run only once (after Add Status Conditions for FAR CR #69), then when it will be run again on a new node it won't try to execute the FA command. Therefore, for this scenario we are skipping the check of checkFarLogs function.