🐛 Machine deletion: try up to 10s to delete the Node, then move on #1452
Conversation
k8s-ci-robot
added
cncf-cla: yes
tahsinrahman
force-pushed
the
fix-delete-machine
branch
3 times, most recently
from
3119c2c
to
ac36bf3
Compare
tahsinrahman
force-pushed
the
fix-delete-machine
branch
from
ac36bf3
to
1baaf27
Compare
tahsinrahman
force-pushed
the
fix-delete-machine
branch
from
1baaf27
to
51325d5
Compare
| return ctrl.Result{}, err | ||
| waitErr := wait.PollImmediate(2*time.Second, 10*time.Second, func() (bool, error) { | ||
| if err = r.deleteNode(ctx, cluster, m.Status.NodeRef.Name); err != nil && !apierrors.IsNotFound(err) { | ||
| return false, nil |
There was a problem hiding this comment.
Maybe we should record the error as deleteNodeErr and report that back to the user in the event instead of waitErr (I fear waitErr will be "timed out waiting for condition")?
There was a problem hiding this comment.
yes, waitErr will be "timed out waiting for condition"
do you mean doing this deleteNodeErr = r.deleteNode..... ? and keeping waitErr as it is?
tahsinrahman
force-pushed
the
fix-delete-machine
branch
from
51325d5
to
09bf517
Compare
controllers/machine_controller.go
Outdated
|
|
||
| var deleteNodeErr error | ||
| err = wait.PollImmediate(2*time.Second, 10*time.Second, func() (bool, error) { | ||
| if deleteNodeErr = r.deleteNode(ctx, cluster, m.Status.NodeRef.Name); err != nil && !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
you are assigning to deleteNodeErr and then testing against err
controllers/machine_controller.go
Outdated
| } | ||
| return true, nil | ||
| }) | ||
| if deleteNodeErr != nil { |
There was a problem hiding this comment.
Won't this also trigger if apierrors.IsNotFound(deleteNodeErr) is true giving a false error?
tahsinrahman
force-pushed
the
fix-delete-machine
branch
from
09bf517
to
a396f02
Compare
| if waitErr != nil { | ||
| // TODO: remove m.Name after #1203 | ||
| r.Log.Error(deleteNodeErr, "timed out deleting Machine's node, moving on", "node", m.Status.NodeRef.Name, "machine", m.Name) | ||
| r.recorder.Eventf(m, corev1.EventTypeWarning, "FailedDeleteNode", "error deleting Machine's node: %v", deleteNodeErr) |
There was a problem hiding this comment.
I assume the answer is "no" but it's worth asking: the if condition here is a non-nil waitErr. Is there a chance that waitErr is not nil, but deleteNodeErr is nil (since we're including deleteNodeErr in the event)?
There was a problem hiding this comment.
no, if waitErr is not nil, deleteNodeErr will definitely not-nil
if deleteNodeErr is nil, then conditionFunc will return true and waitErr will be nil
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc, tahsinrahman 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 |
k8s-ci-robot
added
the
approved
|
Whoops thanks, I assumed you approved without looking :D |
|
No, mistake on my part. I meant to /approve and then assign to you. |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #1446