storage: make cmdTimeout configurable via plugin config

[WaylandYang]

Implements proposal (1) from #235.

Adds a new optional cmd_timeout field to the storage plugin's TOML config. When unset (or zero) the existing 3 s default is preserved exactly — no behavior change for current deployments.

Motivation

shell.go's cp / truncate / e2fsck / resize2fs / mkfs.ext4 chain shares a single 3 s timeout per command. On the pre-formatted pool fast path the small images comfortably finish inside 3 s. On the live-create slow path (writable_layer_size that doesn't match pool_default_format_size_list), 3 s on multi-GiB images is tight under concurrent load — that's the contention window where the reflink-copy race in #235 surfaces.

Letting operators raise the per-command timeout from config is the smallest knob that lets the live-create path survive without recompiling cubelet.

Config example

[plugins."io.cubelet.internal.v1.storage"]
  cmd_timeout = "30s"

What's deliberately not changed

• Default value stays 3 s. Bumping the default is a behavior change you may want to do separately after measuring P95 on a host that actually hits the slow path.
• Single timeout shared by all the commands. Splitting per-command (e.g. faster cp, slower e2fsck) is doable but expands scope; happy to do as a follow-up PR if you'd like.
• shell.go's cmdTimeout is now var instead of const, set during (*local).init. Other styles (passing timeout into each function, plumbing through a context) are also options; this matches the existing package-level state in this package (localStorage, defaultPoolSize, ...).

Out-of-scope observations

• shell.go's three create-image functions (newExt4BaseRaw, newExt4RawByCopy, newExt4RawByReflinkCopy) all swallow stderr into a short string error without command index / file stat / elapsed time. Proposal (2) from Cubelet/storage: newExt4RawByReflinkCopy fails with "bad magic number in superblock" under N=100 concurrent spawn #235 covers that — separate PR coming, keeping this one focused.

Tested

• go vet ./storage/... — clean
• go build ./storage/... — clean
• gofmt -l — clean

[fslongjin]

/cubebot review

[github-actions]

Claude finished @fslongjin's task in 3m 11s —— View job

---

PR Review: storage: make cmdTimeout configurable via plugin config

• Gather context: read changed files and diff
• Analyze implementation for bugs, security, and correctness
• Post review feedback

---

Overall: The approach is clean and minimal — a tomlext.Duration config field, zero-means-default, package-level var set once during init. This matches existing patterns (ReconcileInterval, defaultPoolSize, etc.). A few things to consider:

---

Bug: cmdTimeout is not applied during checkPoolType

checkPoolType calls newExt4BaseRaw and newExt4RawByReflinkCopy (both use cmdTimeout), but it runs before localStorage.init(ic), which is where cmdTimeout gets set from config:

plugin.go:84-97 (init order):

checkPoolType(localStorage.config)   // ← uses cmdTimeout, but it's still 3s here
...
if err := localStorage.init(ic); err != error {  // ← cmdTimeout is set here

local.go:182-185 (where it's actually set):

if l.config.CmdTimeout == 0 {
    l.config.CmdTimeout = tomlext.FromStdTime(defaultCmdTimeout)
}
cmdTimeout = tomlext.ToStdTime(l.config.CmdTimeout)

So during checkPoolType, the user's cmd_timeout config is silently ignored and the 3s default is used instead. In practice the probe image is tiny (512000 bytes), so 3s is likely fine — but it's still a logical gap that could surprise operators who set a high timeout expecting it to apply everywhere.

Suggested fix: Move the cmdTimeout assignment earlier, before checkPoolType:

Fix this →

---

Minor: No validation for negative durations

tomlext.Duration.UnmarshalText will happily parse "-1s", producing a negative time.Duration. A negative timeout passed to context.WithTimeout immediately expires the context, making every shell command fail silently at startup. Consider adding a guard:

if l.config.CmdTimeout < 0 {
    return fmt.Errorf("cmd_timeout must be non-negative, got %v", l.config.CmdTimeout)
}

This is low severity — operators are unlikely to misconfigure this — but a single comparison prevents a confusing failure mode.

---

Nit: Comment in shell.go references local.init

The comment on cmdTimeout says "set by (l *local).init", which is accurate today but creates a subtle coupling — if someone moves the assignment (e.g. to fix the checkPoolType ordering above), the comment becomes stale. Consider a more generic phrasing like "set during plugin initialization" rather than naming a specific function.

---

What looks good

• Backward compatibility: Zero/unset falls through to the existing 3s default. No behavior change for current deployments.
• Consistent pattern: Matches how ReconcileInterval and other config fields handle defaults.
• var instead of const: Pragmatic choice, consistent with localStorage, defaultPoolSize, etc.
• tomlext.Duration: Reuses the existing TOML duration type with ParseDuration semantics — "30s", "5m", etc. work naturally.
• PR scope: Focused on the single knob. Per-command timeouts and better error diagnostics correctly deferred.

[hlz260]

I think it would be more appropriate to move this initialization code to the InitFn in plugin.go

[WaylandYang]

Thanks @hlz260 — good catch, same ordering issue cubebot flagged. Pushed an update that moves cmdTimeout init into InitFn before checkPoolType, plus a non-negative guard and the comment cleanup cubebot also suggested. PTAL.

[hlz260]

LGTM