Skip to content

snapshots/proxy: add optional default_timeout for proxy snapshotter calls - #14187

Merged
fuweid merged 1 commit into
containerd:mainfrom
NahumLitvin:proxy-snapshotter-default-timeout
Sep 30, 2026
Merged

fuweid merged 1 commit into
containerd:mainfrom
NahumLitvin:proxy-snapshotter-default-timeout

Conversation

@NahumLitvin

@NahumLitvin NahumLitvin commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A proxy snapshotter runs in its own process and none of its 10 gRPC methods get a deadline unless the caller already set one. Snapshot GC calls Walk and then Remove per snapshot while holding the snapshotter-level lock in garbageCollect, so one wedged call blocks every later mutating call on that snapshotter and no new container using it can start. #14010 lists the other daemon paths with the same shape.

#13799 and #14026 each bounded one component. Per @fuweid in #14026, the bound belongs at the client instead, where it covers all 10 methods at once. @mikebrow agreed on 09/02. #13799 was reverted in #14119 ahead of 2.4 to go this route.

Fix

New default_timeout on [proxy_plugins]. When set, each proxy snapshotter call gets that deadline if the incoming context has none. A context that already carries its own deadline is left as it is, so this does not shorten anyone's request budget. Empty or unset keeps current behavior.

The option goes through a new NewSnapshotterWithOpts, so the exported NewSnapshotter signature is unchanged for package users.

Not in here

Total pass duration is still unbounded: N snapshots each finishing just under the limit still add up while garbageCollect holds the lock. That is @mikebrow's second point from the #14026 thread and it needs the lock itself reworked, so I left it out.

The content store proxy has the same gap, but it does not fit this shape. Writer outlives the call that created it, so a per-call deadline would cut an ingest that is still being written. It needs its own key and its own PR.

Walk gets one deadline for the whole stream, not one per received message, so time spent in callbacks counts against it and the next Recv fails once it expires. It cannot interrupt a callback that ignores its context. The GC callback only records keys, the call that wedges is the proxy RPC. Say the word if you would rather Walk and Cleanup were left out.

Tests

TestWithTimeout covers both guards. Drop the defaultTimeout <= 0 check and the disabled case goes red; drop the ctx.Deadline() check and the case that preserves a caller's 2h deadline goes red.

TestRemoveAppliesDefaultTimeout goes through NewSnapshotterWithOpts against a client that blocks until its context is done, so deleting the wrapper from Remove fails it in 5s with a clear message instead of hanging the package.

TestLoadPluginsRejectsMalformedProxyDefaultTimeout covers the config path, where a bad duration has to fail daemon startup rather than be ignored. Only the error path is exercised: LoadPlugins returns before it reaches the global plugin registry, which panics if a test registers the same id twice.

Refs #14010. Supersedes #14026.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Preserve the original context for Walk callbacks while bounding only the gRPC stream.

Pull request overview

Adds configurable default deadlines for proxy snapshotter gRPC calls while preserving caller-provided deadlines.

Changes:

  • Adds default_timeout configuration.
  • Applies optional timeouts across snapshotter methods.
  • Wires configuration, tests, and documentation.
File summaries
File Summary
docs/man/containerd-config.toml.5.md Documents default_timeout.
core/snapshots/proxy/proxy.go Adds configurable call timeouts.
core/snapshots/proxy/proxy_test.go Tests timeout behavior.
cmd/containerd/server/server.go Parses and passes timeout configuration.
cmd/containerd/server/config/config.go Defines the timeout setting.
Review details

Suppressed comments (1)

core/snapshots/proxy/proxy.go:211

  • The derived timeout context is also passed to Walk callbacks below, so callbacks receive a deadline that the caller did not provide. This changes the callback contract compared with other snapshotter implementations and can stop callback work at the proxy RPC timeout even though only the remote List stream should be bounded. Keep the original incoming context for fn while using a separate derived context for the gRPC stream.
	ctx, cancel := p.withTimeout(ctx)
	defer cancel()
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Reject non-positive configured durations and strengthen the timeout assertion.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

cmd/containerd/server/server.go:336

  • A configured non-empty duration such as 0s or -1s parses successfully, but WithDefaultTimeout treats it as disabled, so a typo can silently leave all proxy calls unbounded even though default_timeout is set. Reject non-positive durations during config loading instead of installing an option that disables the safeguard.
			if pp.DefaultTimeout != "" {
				d, perr := time.ParseDuration(pp.DefaultTimeout)
				if perr != nil {
					return nil, fmt.Errorf("proxy plugin %q: unable to parse default_timeout %q into a time duration: %w", name, pp.DefaultTimeout, perr)
				}
				ssopts = append(ssopts, ssproxy.WithDefaultTimeout(d))
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread core/snapshots/proxy/proxy_test.go Outdated
Copilot AI review requested due to automatic review settings September 17, 2026 15:43
@NahumLitvin
NahumLitvin force-pushed the proxy-snapshotter-default-timeout branch from e4617b2 to 350ade8 Compare September 17, 2026 15:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved blocking issues were identified.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 21, 2026 09:27
@NahumLitvin
NahumLitvin force-pushed the proxy-snapshotter-default-timeout branch from 350ade8 to 9400c17 Compare September 21, 2026 09:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: 1 Low severity

Open (1)

@NahumLitvin
NahumLitvin force-pushed the proxy-snapshotter-default-timeout branch from 9400c17 to 652b842 Compare September 22, 2026 07:15
Copilot AI review requested due to automatic review settings September 22, 2026 07:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Walk callbacks can still block beyond the RPC deadline while holding the snapshotter lock.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Comment thread core/snapshots/proxy/proxy.go

@fuweid fuweid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM. please address one comment. thanks

Comment thread core/snapshots/proxy/proxy.go Outdated
Copilot AI review requested due to automatic review settings September 26, 2026 05:52
@NahumLitvin
NahumLitvin force-pushed the proxy-snapshotter-default-timeout branch from 652b842 to eacccdd Compare September 26, 2026 05:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Outstanding review comments request constructor API alignment and broader per-method deadline coverage.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)

Comment thread core/snapshots/proxy/proxy.go
@NahumLitvin
NahumLitvin force-pushed the proxy-snapshotter-default-timeout branch from eacccdd to e3fc501 Compare September 28, 2026 10:38

@fuweid fuweid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikebrow mikebrow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple nits to consider on the documentation

Comment thread docs/man/containerd-config.toml.5.md Outdated
Comment thread docs/PLUGINS.md Outdated
Comment thread docs/PLUGINS.md Outdated
…alls

A proxy snapshotter is a separate process, and none of its 10 gRPC
methods carry a deadline unless the caller already set one. Snapshot GC
calls Walk and Remove under the snapshotter-level lock, so one wedged
call blocks every later mutating call on that snapshotter.

Add default_timeout to the proxy_plugins config section. When set, each
proxy snapshotter call gets that deadline if the incoming context has
none; a context that already carries its own deadline is left alone.
Empty or unset keeps today's behavior, so nothing changes for existing
configs.

NewSnapshotter keeps its signature and wraps the new
NewSnapshotterWithOpts, so existing callers of the exported constructor
are unaffected.

Assisted-by: Claude Code
Signed-off-by: Nahum Litvin <nahuml@wix.com>
Copilot AI review requested due to automatic review settings September 29, 2026 11:17
@NahumLitvin
NahumLitvin force-pushed the proxy-snapshotter-default-timeout branch from e3fc501 to 2a843ba Compare September 29, 2026 11:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The remaining test-strengthening suggestion is a minor nit and does not block approval.

Review effort: Lite
Findings: None

Resolved since last review (2)

@mikebrow mikebrow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lGTM

@fuweid
fuweid added this pull request to the merge queue Sep 30, 2026
Merged via the queue into containerd:main with commit fcb8ec6 Sep 30, 2026
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Development

Successfully merging this pull request may close these issues.

4 participants