daemon: NewDaemon: align grpc options with containerd's defaults#48617
Merged
vvoland merged 6 commits intomoby:masterfrom Jan 21, 2025
Merged
daemon: NewDaemon: align grpc options with containerd's defaults#48617vvoland merged 6 commits intomoby:masterfrom
vvoland merged 6 commits intomoby:masterfrom
Conversation
vvoland
approved these changes
Oct 17, 2024
thaJeztah
force-pushed
the
update_grpc_options
branch
2 times, most recently
from
e5b95e9 to
76617ef
Compare
thaJeztah
force-pushed
the
update_grpc_options
branch
from
76617ef to
4665b77
Compare
thaJeztah
force-pushed
the
update_grpc_options
branch
from
4665b77 to
30005a5
Compare
thaJeztah
commented
Jan 16, 2025
| // ------------------------------------------------------------------ | ||
| // options below are copied from containerd client's default options | ||
| // | ||
| // TODO(thaJeztah): update this list once https://github.com/containerd/containerd/pull/10250/commits/63b46881753588624b2eac986660458318581330 is in the 1.7 release. |
Member
Author
There was a problem hiding this comment.
Oh! Maybe I can remove more now, because we switched to the containerd v2 module; let me check before we merge.
Dial-options passed to containerd _override_ all defaults that are set in containerd, and containerd does not provide an option to provide the defaults in other ways, which makes it slightly more complicated to use the defaults combined with some custom options. https://github.com/containerd/containerd/blob/v1.7.22/client.go#L122-L132 This patch aligns the options we set with the defaults in containerd. grpc.FailOnNonTempDialError was added together with WithBlock in [containerd@64bc516], but it looks like this was not copied to our options when the equivalent was added in this repository through 9f73396. grpc.WithReturnConnectionError was added in [containerd@73d28dd] to improve handling of connection errors; Permission errors: % ./bin/ctr t ls ctr: failed to dial "/run/containerd/containerd.sock": connection error: desc = "transport: error while dialing: dial unix /run/containerd/containerd.sock: connect: permission denied" % Non-existent sockets: % ./bin/ctr -a notfound t ls ctr: failed to dial "notfound": context deadline exceeded: connection error: desc = "transport: error while dialing: dial unix://notfound: timeout" % That commit failed to notice that WithReturnConnectionError implies WithBlock, so removing that option from the list. Note that both WithBlock and WithReturnConnectionError are deprecated in newer versions of grpc, so we should remove these once [containerd@63b4688] makes it into the containerd 1.7 branch (and vendored). [containerd@64bc516]: containerd/containerd@64bc516 [containerd@73d28dd]: containerd/containerd@73d28dd [containerd@63b4688]: containerd/containerd@63b4688 Signed-off-by: Sebastiaan van Stijn <[email protected]>
The default message size is set unconditionally in containerd's client, so unlike Dial-options, there's no risk of implicitly dropping these options. TThis patch removes the options, as they were the same as the default already set in containerd itself. https://github.com/containerd/containerd/blob/v1.7.22/client.go#L133-L138 Signed-off-by: Sebastiaan van Stijn <[email protected]>
The default message size is set unconditionally in containerd's client, so unlike Dial-options, there's no risk of implicitly dropping these options. TThis patch removes the options, as they were the same as the default already set in containerd itself. https://github.com/containerd/containerd/blob/v1.7.22/client.go#L133-L138 Signed-off-by: Sebastiaan van Stijn <[email protected]>
These were moved in 84965c0 Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
update_grpc_options
branch
from
30005a5 to
f0041c7
Compare
thaJeztah
added a commit
to thaJeztah/containerd
that referenced
this pull request
Jan 17, 2025
the client package provides a WithDialOpts option, however, dial-options passed to override all defaults that are set in containerd. This makes it difficult to expand the defaults with custom options, as this requires copying the defaults, and trying to keep those in sync (e.g. see [moby#48617]). This patch introduces a new `WithExtraDialOpts` option which, unlike `WithDialOpts` are appended to, instead of overriding, previous options. This allows setting custom options, while maintaining containerd's defaults. Also unlike `WithDialOpts`, this option can be used multiple times to allow additional options to be set. [moby#48617]: moby/moby#48617 Signed-off-by: Sebastiaan van Stijn <[email protected]>
Now that we moved to use containerd 2.0, the changes from containerd/containerd@63b4688 can now be used, removing some of gRPC's deprecated options. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
update_grpc_options
branch
from
f0041c7 to
cd14e68
Compare
Member
Author
|
OK; rebased and updated for containerd 2.0 in the last two commits. I also opened a PR in containerd to make this less painful to keep in sync; @vvoland @laurazard @dmcgowan PTAL |
thaJeztah
added a commit
to thaJeztah/containerd
that referenced
this pull request
Jan 19, 2025
the client package provides a WithDialOpts option, however, dial-options passed to override all defaults that are set in containerd. This makes it difficult to expand the defaults with custom options, as this requires copying the defaults, and trying to keep those in sync (e.g. see [moby#48617]). This patch introduces a new `WithExtraDialOpts` option which, unlike `WithDialOpts` are appended to, instead of overriding, previous options. This allows setting custom options, while maintaining containerd's defaults. Also unlike `WithDialOpts`, this option can be used multiple times to allow additional options to be set. [moby#48617]: moby/moby#48617 Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
commented
Jan 20, 2025
Comment on lines
913
to
917
| // Set the max backoff delay to match our containerd.WithTimeout(), | ||
| // aligning with how containerd client's defaults sets this; | ||
| // https://github.com/containerd/containerd/blob/v2.0.2/client/client.go#L129-L136 | ||
| backoffConfig := backoff.DefaultConfig | ||
| backoffConfig.MaxDelay = 3 * time.Second |
Member
Author
There was a problem hiding this comment.
Of course, it's better to not only change the comment, but also the code 😂 🙈
Let me fix this
containerd 1.7 and lower set this value to a fixed, 3-second delay; https://github.com/containerd/containerd/blob/v1.7.22/client.go#L117-L121 containerd 2.0 (starting with [containerd@63b4688]) aligned this value with clientopts.timeout (WithTimeout()), which we set to 60 seconds; https://github.com/containerd/containerd/blob/v2.0.2/client/client.go#L129-L136 This patch aligns our code with containerd client's defaults. [containerd@63b4688]: containerd/containerd@63b4688 Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
update_grpc_options
branch
from
cd14e68 to
131441b
Compare
thaJeztah
added a commit
to thaJeztah/containerd
that referenced
this pull request
Jan 21, 2025
the client package provides a WithDialOpts option, however, dial-options passed to override all defaults that are set in containerd. This makes it difficult to expand the defaults with custom options, as this requires copying the defaults, and trying to keep those in sync (e.g. see [moby#48617]). This patch introduces a new `WithExtraDialOpts` option which, unlike `WithDialOpts` are appended to, instead of overriding, previous options. This allows setting custom options, while maintaining containerd's defaults. Also unlike `WithDialOpts`, this option can be used multiple times to allow additional options to be set. [moby#48617]: moby/moby#48617 Signed-off-by: Sebastiaan van Stijn <[email protected]>
vvoland
approved these changes
Jan 21, 2025
sreeram-venkitesh
pushed a commit
to sreeram-venkitesh/containerd
that referenced
this pull request
Feb 3, 2025
the client package provides a WithDialOpts option, however, dial-options passed to override all defaults that are set in containerd. This makes it difficult to expand the defaults with custom options, as this requires copying the defaults, and trying to keep those in sync (e.g. see [moby#48617]). This patch introduces a new `WithExtraDialOpts` option which, unlike `WithDialOpts` are appended to, instead of overriding, previous options. This allows setting custom options, while maintaining containerd's defaults. Also unlike `WithDialOpts`, this option can be used multiple times to allow additional options to be set. [moby#48617]: moby/moby#48617 Signed-off-by: Sebastiaan van Stijn <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
daemon: NewDaemon: align grpc options with containerd's defaults
Dial-options passed to containerd override all defaults that are set
in containerd, and containerd does not provide an option to provide
the defaults in other ways, which makes it slightly more complicated
to use the defaults combined with some custom options.
https://github.com/containerd/containerd/blob/v1.7.22/client.go#L122-L132
This patch aligns the options we set with the defaults in containerd.
grpc.FailOnNonTempDialError was added together with WithBlock in containerd@64bc516,
but it looks like this was not copied to our options when the equivalent was
added in this repository through 9f73396.
grpc.WithReturnConnectionError was added in containerd@73d28dd to improve
handling of connection errors;
Permission errors:
Non-existent sockets:
That commit failed to notice that WithReturnConnectionError implies WithBlock,
so removing that option from the list.
Note that both WithBlock and WithReturnConnectionError are deprecated in
newer versions of grpc, so we should remove these once containerd@63b4688
makes it into the containerd 1.7 branch (and vendored).
daemon: NewDaemon: remove grpc options that are the default
The default message size is set unconditionally in containerd's client,
so unlike Dial-options, there's no risk of implicitly dropping these
options.
TThis patch removes the options, as they were the same as the default
already set in containerd itself.
https://github.com/containerd/containerd/blob/v1.7.22/client.go#L133-L138
libcontainerd/supervisor: remove grpc options that are the default
The default message size is set unconditionally in containerd's client,
so unlike Dial-options, there's no risk of implicitly dropping these
options.
TThis patch removes the options, as they were the same as the default
already set in containerd itself.
https://github.com/containerd/containerd/blob/v1.7.22/client.go#L133-L138
- A picture of a cute animal (not mandatory but encouraged)