-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add support for request timeout propgation. #31
Conversation
Side note, this explicitly does not support client-side context cancellation, which will require more effort. |
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
I think this fits the bill of the design.
Looking at it and back at the grpc wire protocol and the design of ttrpc, I am wondering if we really need to support generic metadata or can we just add a Deadline field. I may have been pretty wrong to suggest using the metadata fields to perform that, but its clear there is a cost of allocations (the map, the values wrapper, the slice and the value itself) over just having a field. Its up to you if you want to change to the simpler model and I'm sorry for the run around.
I would like to support tracing propagation as well. So metadata doesn't seem bad... but again maybe dedicated fields would indeed help with allocations. Was also wondering if this should be done with the interceptor pattern rather than automatically adding it. |
I think automating is fine. All of the grpc packages do it and it makes it a no brainer for users. Yea, I think metadata is needed, but maybe not for deadline. It would be nice to omit the allocation when its not in use. |
ce52476
to
1660934
Compare
1660934
to
54101cc
Compare
Adds a new field to the `Request` type which specifies a timeout (in nanoseconds) for the request. This is propagated on method dispatch as a context timeout. There was some discussion here on supporting a broader "metadata" field (similar to grpc) that can be used for other things, but we ended up with a dedicated field because it is lighter weight and expect it to be used pretty heavily as is.... metadata may be added in the future, but is not necessary for timeouts. Also discussed using a deadline vs a timeout in the request and decided to go with a timeout in order to deal with potential clock skew between the client and server. This also has the side-effect of eliminating the protocol/wire overhead from the request timeout. Signed-off-by: Brian Goff <[email protected]>
54101cc
to
a364f44
Compare
After discussing this on Slack, decided to:
This PR has been updated with the requested changes. |
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
Let's get a test into containerd to make sure this is working as intended. PTAL @dmcgowan @crosbymichael |
From the release notes: https://github.com/containerd/containerd/releases/tag/v1.2.7 > Welcome to the v1.2.7 release of containerd! > > The seventh patch release for containerd 1.2 introduces OCI image > descriptor annotation support and contains fixes for containerd shim logs, > container stop/deletion, cri plugin and selinux. > > It also contains several important bug fixes for goroutine and file > descriptor leakage in containerd and containerd shims. > > Notable Updates > > - Support annotations in the OCI image descriptor, and filtering image by annotations. containerd/containerd#3254 > - Support context timeout in ttrpc which can help avoid containerd hangs when a shim is unresponsive. containerd/ttrpc#31 > - Fix a bug that containerd shim leaks goroutine and file descriptor after containerd restarts. containerd/ttrpc#37 > - Fix a bug that a container can't be deleted if first deletion attempt is canceled or timeout. containerd/containerd#3264 > - Fix a bug that containerd leaks file descriptor when using v2 containerd shims, e.g. containerd-shim-runc-v1. containerd/containerd#3273 > - Fix a bug that a container with lingering processes can't terminate when it shares pid namespace with another container. moby#38978 > - Fix a bug that containerd can't read shim logs after restart. containerd/containerd#3282 > - Fix a bug that shim_debug option is not honored for existing containerd shims after containerd restarts. containerd/containerd#3283 > - cri: Fix a bug that a container can't be stopped when the exit event is not successfully published by the containerd shim. containerd/containerd#3125, containerd/containerd#3177 > - cri: Fix a bug that exec process is not cleaned up if grpc context is canceled or timeout. contaienrd/cri#1159 > - Fix a selinux keyring labeling issue by updating runc to v1.0.0-rc.8 and selinux library to v1.2.2. opencontainers/selinux#50 > - Update ttrpc to f82148331ad2181edea8f3f649a1f7add6c3f9c2. containerd/containerd#3316 > - Update cri to 49ca74043390bc2eeea7a45a46005fbec58a3f88. containerd/containerd#3330 Signed-off-by: Sebastiaan van Stijn <[email protected]>
From the release notes: https://github.com/containerd/containerd/releases/tag/v1.2.7 > Welcome to the v1.2.7 release of containerd! > > The seventh patch release for containerd 1.2 introduces OCI image > descriptor annotation support and contains fixes for containerd shim logs, > container stop/deletion, cri plugin and selinux. > > It also contains several important bug fixes for goroutine and file > descriptor leakage in containerd and containerd shims. > > Notable Updates > > - Support annotations in the OCI image descriptor, and filtering image by annotations. containerd/containerd#3254 > - Support context timeout in ttrpc which can help avoid containerd hangs when a shim is unresponsive. containerd/ttrpc#31 > - Fix a bug that containerd shim leaks goroutine and file descriptor after containerd restarts. containerd/ttrpc#37 > - Fix a bug that a container can't be deleted if first deletion attempt is canceled or timeout. containerd/containerd#3264 > - Fix a bug that containerd leaks file descriptor when using v2 containerd shims, e.g. containerd-shim-runc-v1. containerd/containerd#3273 > - Fix a bug that a container with lingering processes can't terminate when it shares pid namespace with another container. moby/moby#38978 > - Fix a bug that containerd can't read shim logs after restart. containerd/containerd#3282 > - Fix a bug that shim_debug option is not honored for existing containerd shims after containerd restarts. containerd/containerd#3283 > - cri: Fix a bug that a container can't be stopped when the exit event is not successfully published by the containerd shim. containerd/containerd#3125, containerd/containerd#3177 > - cri: Fix a bug that exec process is not cleaned up if grpc context is canceled or timeout. contaienrd/cri#1159 > - Fix a selinux keyring labeling issue by updating runc to v1.0.0-rc.8 and selinux library to v1.2.2. opencontainers/selinux#50 > - Update ttrpc to f82148331ad2181edea8f3f649a1f7add6c3f9c2. containerd/containerd#3316 > - Update cri to 49ca74043390bc2eeea7a45a46005fbec58a3f88. containerd/containerd#3330 Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: d5669ec1c6eedcd5dd8b0ecd615638934561daa4 Component: engine
From the release notes: https://github.com/containerd/containerd/releases/tag/v1.2.7 > Welcome to the v1.2.7 release of containerd! > > The seventh patch release for containerd 1.2 introduces OCI image > descriptor annotation support and contains fixes for containerd shim logs, > container stop/deletion, cri plugin and selinux. > > It also contains several important bug fixes for goroutine and file > descriptor leakage in containerd and containerd shims. > > Notable Updates > > - Support annotations in the OCI image descriptor, and filtering image by annotations. containerd/containerd#3254 > - Support context timeout in ttrpc which can help avoid containerd hangs when a shim is unresponsive. containerd/ttrpc#31 > - Fix a bug that containerd shim leaks goroutine and file descriptor after containerd restarts. containerd/ttrpc#37 > - Fix a bug that a container can't be deleted if first deletion attempt is canceled or timeout. containerd/containerd#3264 > - Fix a bug that containerd leaks file descriptor when using v2 containerd shims, e.g. containerd-shim-runc-v1. containerd/containerd#3273 > - Fix a bug that a container with lingering processes can't terminate when it shares pid namespace with another container. moby#38978 > - Fix a bug that containerd can't read shim logs after restart. containerd/containerd#3282 > - Fix a bug that shim_debug option is not honored for existing containerd shims after containerd restarts. containerd/containerd#3283 > - cri: Fix a bug that a container can't be stopped when the exit event is not successfully published by the containerd shim. containerd/containerd#3125, containerd/containerd#3177 > - cri: Fix a bug that exec process is not cleaned up if grpc context is canceled or timeout. contaienrd/cri#1159 > - Fix a selinux keyring labeling issue by updating runc to v1.0.0-rc.8 and selinux library to v1.2.2. opencontainers/selinux#50 > - Update ttrpc to f82148331ad2181edea8f3f649a1f7add6c3f9c2. containerd/containerd#3316 > - Update cri to 49ca74043390bc2eeea7a45a46005fbec58a3f88. containerd/containerd#3330 Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit d5669ec) Signed-off-by: Sebastiaan van Stijn <[email protected]>
From the release notes: https://github.com/containerd/containerd/releases/tag/v1.2.7 > Welcome to the v1.2.7 release of containerd! > > The seventh patch release for containerd 1.2 introduces OCI image > descriptor annotation support and contains fixes for containerd shim logs, > container stop/deletion, cri plugin and selinux. > > It also contains several important bug fixes for goroutine and file > descriptor leakage in containerd and containerd shims. > > Notable Updates > > - Support annotations in the OCI image descriptor, and filtering image by annotations. containerd/containerd#3254 > - Support context timeout in ttrpc which can help avoid containerd hangs when a shim is unresponsive. containerd/ttrpc#31 > - Fix a bug that containerd shim leaks goroutine and file descriptor after containerd restarts. containerd/ttrpc#37 > - Fix a bug that a container can't be deleted if first deletion attempt is canceled or timeout. containerd/containerd#3264 > - Fix a bug that containerd leaks file descriptor when using v2 containerd shims, e.g. containerd-shim-runc-v1. containerd/containerd#3273 > - Fix a bug that a container with lingering processes can't terminate when it shares pid namespace with another container. moby/moby#38978 > - Fix a bug that containerd can't read shim logs after restart. containerd/containerd#3282 > - Fix a bug that shim_debug option is not honored for existing containerd shims after containerd restarts. containerd/containerd#3283 > - cri: Fix a bug that a container can't be stopped when the exit event is not successfully published by the containerd shim. containerd/containerd#3125, containerd/containerd#3177 > - cri: Fix a bug that exec process is not cleaned up if grpc context is canceled or timeout. contaienrd/cri#1159 > - Fix a selinux keyring labeling issue by updating runc to v1.0.0-rc.8 and selinux library to v1.2.2. opencontainers/selinux#50 > - Update ttrpc to f82148331ad2181edea8f3f649a1f7add6c3f9c2. containerd/containerd#3316 > - Update cri to 49ca74043390bc2eeea7a45a46005fbec58a3f88. containerd/containerd#3330 Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit d5669ec1c6eedcd5dd8b0ecd615638934561daa4) Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 768923199f89246ff51039ae030e4b492f8d4555 Component: engine
From the release notes: https://github.com/containerd/containerd/releases/tag/v1.2.7 > Welcome to the v1.2.7 release of containerd! > > The seventh patch release for containerd 1.2 introduces OCI image > descriptor annotation support and contains fixes for containerd shim logs, > container stop/deletion, cri plugin and selinux. > > It also contains several important bug fixes for goroutine and file > descriptor leakage in containerd and containerd shims. > > Notable Updates > > - Support annotations in the OCI image descriptor, and filtering image by annotations. containerd/containerd#3254 > - Support context timeout in ttrpc which can help avoid containerd hangs when a shim is unresponsive. containerd/ttrpc#31 > - Fix a bug that containerd shim leaks goroutine and file descriptor after containerd restarts. containerd/ttrpc#37 > - Fix a bug that a container can't be deleted if first deletion attempt is canceled or timeout. containerd/containerd#3264 > - Fix a bug that containerd leaks file descriptor when using v2 containerd shims, e.g. containerd-shim-runc-v1. containerd/containerd#3273 > - Fix a bug that a container with lingering processes can't terminate when it shares pid namespace with another container. moby#38978 > - Fix a bug that containerd can't read shim logs after restart. containerd/containerd#3282 > - Fix a bug that shim_debug option is not honored for existing containerd shims after containerd restarts. containerd/containerd#3283 > - cri: Fix a bug that a container can't be stopped when the exit event is not successfully published by the containerd shim. containerd/containerd#3125, containerd/containerd#3177 > - cri: Fix a bug that exec process is not cleaned up if grpc context is canceled or timeout. contaienrd/cri#1159 > - Fix a selinux keyring labeling issue by updating runc to v1.0.0-rc.8 and selinux library to v1.2.2. opencontainers/selinux#50 > - Update ttrpc to f82148331ad2181edea8f3f649a1f7add6c3f9c2. containerd/containerd#3316 > - Update cri to 49ca74043390bc2eeea7a45a46005fbec58a3f88. containerd/containerd#3330 Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit d5669ec) Signed-off-by: Sebastiaan van Stijn <[email protected]>
From the release notes: https://github.com/containerd/containerd/releases/tag/v1.2.7 > Welcome to the v1.2.7 release of containerd! > > The seventh patch release for containerd 1.2 introduces OCI image > descriptor annotation support and contains fixes for containerd shim logs, > container stop/deletion, cri plugin and selinux. > > It also contains several important bug fixes for goroutine and file > descriptor leakage in containerd and containerd shims. > > Notable Updates > > - Support annotations in the OCI image descriptor, and filtering image by annotations. containerd/containerd#3254 > - Support context timeout in ttrpc which can help avoid containerd hangs when a shim is unresponsive. containerd/ttrpc#31 > - Fix a bug that containerd shim leaks goroutine and file descriptor after containerd restarts. containerd/ttrpc#37 > - Fix a bug that a container can't be deleted if first deletion attempt is canceled or timeout. containerd/containerd#3264 > - Fix a bug that containerd leaks file descriptor when using v2 containerd shims, e.g. containerd-shim-runc-v1. containerd/containerd#3273 > - Fix a bug that a container with lingering processes can't terminate when it shares pid namespace with another container. moby/moby#38978 > - Fix a bug that containerd can't read shim logs after restart. containerd/containerd#3282 > - Fix a bug that shim_debug option is not honored for existing containerd shims after containerd restarts. containerd/containerd#3283 > - cri: Fix a bug that a container can't be stopped when the exit event is not successfully published by the containerd shim. containerd/containerd#3125, containerd/containerd#3177 > - cri: Fix a bug that exec process is not cleaned up if grpc context is canceled or timeout. contaienrd/cri#1159 > - Fix a selinux keyring labeling issue by updating runc to v1.0.0-rc.8 and selinux library to v1.2.2. opencontainers/selinux#50 > - Update ttrpc to f82148331ad2181edea8f3f649a1f7add6c3f9c2. containerd/containerd#3316 > - Update cri to 49ca74043390bc2eeea7a45a46005fbec58a3f88. containerd/containerd#3330 Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit d5669ec1c6eedcd5dd8b0ecd615638934561daa4) Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 8c7928adaa83264947b6e296eeb068b99843822e Component: engine
From the release notes: https://github.com/containerd/containerd/releases/tag/v1.2.7 > Welcome to the v1.2.7 release of containerd! > > The seventh patch release for containerd 1.2 introduces OCI image > descriptor annotation support and contains fixes for containerd shim logs, > container stop/deletion, cri plugin and selinux. > > It also contains several important bug fixes for goroutine and file > descriptor leakage in containerd and containerd shims. > > Notable Updates > > - Support annotations in the OCI image descriptor, and filtering image by annotations. containerd/containerd#3254 > - Support context timeout in ttrpc which can help avoid containerd hangs when a shim is unresponsive. containerd/ttrpc#31 > - Fix a bug that containerd shim leaks goroutine and file descriptor after containerd restarts. containerd/ttrpc#37 > - Fix a bug that a container can't be deleted if first deletion attempt is canceled or timeout. containerd/containerd#3264 > - Fix a bug that containerd leaks file descriptor when using v2 containerd shims, e.g. containerd-shim-runc-v1. containerd/containerd#3273 > - Fix a bug that a container with lingering processes can't terminate when it shares pid namespace with another container. moby#38978 > - Fix a bug that containerd can't read shim logs after restart. containerd/containerd#3282 > - Fix a bug that shim_debug option is not honored for existing containerd shims after containerd restarts. containerd/containerd#3283 > - cri: Fix a bug that a container can't be stopped when the exit event is not successfully published by the containerd shim. containerd/containerd#3125, containerd/containerd#3177 > - cri: Fix a bug that exec process is not cleaned up if grpc context is canceled or timeout. contaienrd/cri#1159 > - Fix a selinux keyring labeling issue by updating runc to v1.0.0-rc.8 and selinux library to v1.2.2. opencontainers/selinux#50 > - Update ttrpc to f82148331ad2181edea8f3f649a1f7add6c3f9c2. containerd/containerd#3316 > - Update cri to 49ca74043390bc2eeea7a45a46005fbec58a3f88. containerd/containerd#3330 Signed-off-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: zach <[email protected]>
Adds a new field to the
Request
type which specifies a timeout (innanoseconds) for the request. This is propagated on method dispatch as a
context timeout.
There was some discussion here on supporting a broader "metadata" field
(similar to grpc) that can be used for other things, but we ended up
with a dedicated field because it is lighter weight and expect it to be
used pretty heavily as is.... metadata may be added in the future, but
is not necessary for timeouts.
Also discussed using a deadline vs a timeout in the request and decided
to go with a timeout in order to deal with potential clock skew between
the client and server. This also has the side-effect of eliminating the
protocol/wire overhead from the request timeout.