Return GRPC not found error instead of plain one#4860
Return GRPC not found error instead of plain one#4860crosbymichael merged 1 commit intocontainerd:masterfrom
Conversation
|
Hi @danail-branekov. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
danail-branekov
force-pushed
the
pr-process-not-found-err
branch
from
9d58063 to
df8a987
Compare
|
This does not seem like the right place. A quick search for where this might be happening is here: containerd/runtime/v2/runc/v2/service.go Lines 484 to 491 in 3cd1c83 Is that inline with the calling context where you are experiencing this? |
Quite likely, I don't know that piece of the codebase quite well.
Isn't that the function that is getting the state of a task? Note that we are targeting the use case when we ask a task to load a process with non-existing ID. We would be happy with whatever fix as long as it passes the |
When the shim returns a plain error when a process does not exist, the server is unable to recognise its GRPC status code and assumes UnknownError. This is awkward for containerd client users as they are unable to recognise the actual reason for the error. When the shim returns a NotFound GRPC error, it is properly translated by the server and clients receive a proper NotFound error instead of Unknown Please note that we (CF Garden) would like to have the eventual fix backported to 1.4 as well. Co-authored-by: Danail Branekov <[email protected]> Signed-off-by: Danail Branekov <[email protected]> Signed-off-by: Georgi Sabev <[email protected]>
danail-branekov
force-pushed
the
pr-process-not-found-err
branch
from
df8a987 to
7451dd1
Compare
|
@cpuguy83 We have looked at your suggestion more carefully and it makes lots of sense. We have updated the commit to translate the error to GRPC one in Thanks! |
|
Build succeeded.
|
|
/ok-to-test |
|
Unfortunately this isn't fixing the test... and now I'm thinking the test isn't running in CI. |
|
Or maybe it's just my local test env... |
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
LGTM
It was just my test env.
|
LGTM |
|
Thank you for merging the PR. Can we also have it backported to 1.4 please? |
|
Seems fine for a backport. Can you open a pr for it? |
|
Sure, here is the backport PR: #4872 Thanks! |
AkihiroSuda
added
the
cherry-picked/1.4.x
7 participants
When the shim returns a plain error when a process does not exist,
the server is unable to recognise its GRPC status code and assumes
UnknownError. This is awkward for containerd client users as they are
unable to recognise the actual reason for the error.
When the shim returns a NotFound GRPC error, it is properly translated
by the server and clients receive a proper NotFound error instead of
Unknown
Please note that we (CF Garden) would like to have the eventual fix backported to 1.4 as well.
Co-authored-by: Georgi Sabev [email protected]