-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stop Remediation When NHC Timed Out Annotation Exists #72
Conversation
Use commons repo
The annotation is used by Node Healthcheck Operator to stop the remediaiton for escalating remediation scenario
Skipping CI for Draft Pull Request. |
/test 4.13-openshift-e2e |
@@ -129,6 +132,13 @@ func (r *FenceAgentsRemediationReconciler) Reconcile(ctx context.Context, req ct | |||
return emptyResult, nil | |||
} | |||
|
|||
// Check NHC timeout annotation | |||
if isTimedOutByNHC(far) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can go before the if block above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it can
if isTimedOutByNHC(far) { | ||
r.Log.Info("FAR remediation was stopped by Node Healthcheck Operator") | ||
// TODO: update status and return its error | ||
return emptyResult, errors.New(errorNhcTimedOut) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why returning an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that if we wouldn't return an error, then on the next reconcile the CR will be processed and will pass this if on the way to execute FA and delete workloads. But thinking about it again, the CR will be stopped here until the NHC annotation would be removed from the CR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the CR will be stopped here until the NHC annotation would be removed from the CR
exactly, and removing the annotation will trigger a reconcile automatically, no need to requeue ourself 🙂
There is no need to error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
putting a hold on it to let you decide if you want this one to be merged first, or the condition PR 🙂
/hold
[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 |
I prefer that whatever can be merged will be merged. There is no precedence between them. /unhold |
/retest |
1 similar comment
/retest |
Stop remediation when NHC timed out annotation exists
ECOPROJECT-1485