-
Notifications
You must be signed in to change notification settings - Fork 241
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
disk/ad_slot: return better GRPC error message #1218
base: master
Are you sure you want to change the base?
Conversation
We should try our best to return proper error message to client before deadline, even if the request will timeout.
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: huww98 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
align with GRPC itself
expected new message shown in Kubernetes events: failed to reserve node i-xxxx for attach: still waiting for other disk(s) to finish attach/detach Note that I don't specify GRPC status code intensionally, to let GRPC determine the code automatically based on error types.
e59678c
to
d6f9f62
Compare
I've verified that I can see the expected Kubernetes event with this patch. However, this path is hard to trigger, because requests are not enqueued at the same time. when the currently attaching request is cancelled, the next one kicks in and issue AttachDisk call immediately, and likely receive Conflict response. So, these conflicting AttachDisk calls can be avoided. Maybe something like #1190 can be helpful, but still not ideal. Because we don't want to delay the GRPC response to avoid potential timeout. That said, this issue is not introduced by this PR. So I think this one is ready to go. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
expected new message shown in Kubernetes events:
failed to reserve node i-xxxx for attach: still waiting for other disk(s) to finish attach/detach
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: