Simplify the node backoff logic #960
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saikat-royc 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
size/XL
|
/retest |
k8s-ci-robot
added
size/XXL
k8s-ci-robot
added
size/XL
saikat-royc
requested review from
mattcary
and removed request for
davidz627 and
saad-ali
saikat-royc
force-pushed
the
999.0-v1
branch
2 times, most recently
from
3e89e05 to
a274559
Compare
2. backoff on resource exhausted error
|
LGTM |
|
/lgtm |
|
@pwschuurman: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| // Mark the node and rate limit all the following attach/detach | ||
| // operations for this node | ||
| gceCS.publishErrorsSeenOnNode[nodeID] = true | ||
| if gce.IsTooManyRequestError(err) { |
There was a problem hiding this comment.
I think we want to back off on any error. With this logic, we'll let the attacher retry until api quota gets exhausted. I think we would rather not even reach that point.
I don't think there's a case where immediate retries are useful?
| resp, err := gceCS.executeControllerUnpublishVolume(ctx, req) | ||
| backoff := isResourceExhaustedError(err) |
There was a problem hiding this comment.
I agree. I think we should backoff for all errors from executeControllerUnpublishVolume, but we have also seen a customer case where executeControllerUnpublishVolume returns true: ControllerUnpublishVolume succeeded for disk. Already not attached. They still ended up with the same issue where the finalizer external-attacher/pd-csi-storage-gke-io was still set on the VA. I think the CSI attacher was the one that kept retrying because the csi-attacher logs repeated
VA csi-xx for volume projects/xx/zones/us-east4-a/disks/pvc-xx has attached status true but actual state false. Adding back to VA queue for forced reprocessing
Which made the kubecontroller manager keep trying to force detach but failing.
There was a problem hiding this comment.
To elaborate, in some cases,
instance, err := gceCS.CloudProvider.GetInstanceOrError(ctx, instanceZone, instanceName)
will still return the instance , but the device will not show up under devices when we call common.GetDeviceName(volKey), so then controllerUnpublishVolume returns true, but then we never called detachDisk (which I am assuming is what makes VA attached status to be false). So then the csi-attacher keeps queuing the VA. VA csi-xx has attached status true but actual state false. Adding back to VA queue for forced reprocessing
What type of PR is this?
What this PR does / why we need it:
This PR aims to simply the node backoff logic by removing the worker based backoff queue, to a simple map which tracks the active backoff condition per node. Removing the worker, removes dual sources of controller publish/unpublish calls (one from side car, one from the queue worker), and potential out of order execution of attach/detach disks operations.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Tested manually by running multiple cycles of attach/detach operations of 100 disks for a single pod.
Does this PR introduce a user-facing change?: