Skip to content

fix: add --timeout flag to container commit to allow configurable lease timeout - #5253

Merged
ChengyuZhu6 merged 2 commits into
containerd:mainfrom
Luckydog691:fix/commit-timeout-flag
Oct 10, 2026
Merged

ChengyuZhu6 merged 2 commits into
containerd:mainfrom
Luckydog691:fix/commit-timeout-flag

Conversation

@Luckydog691

Copy link
Copy Markdown
Contributor

The hardcoded 1-hour lease expiration in nerdctl container commit can cause failures for containers with large filesystem deltas (e.g., many packages installed). When the diff operation exceeds 1 hour, the containerd GC cleans up the expired lease and its ingest data, resulting in context deadline exceeded / failed to export layer errors.

This PR continues the stalled #5108. The original commit by @Turbo-Jiaxxx789 is preserved as-is (full credit to the original author), rebased onto the latest main, with the outstanding review comment addressed in a follow-up commit.

Changes

  • Add Timeout field to ContainerCommitOptions and commit.Opts
  • Modify Commit() to use a configurable lease expiration:
    • --timeout=0 (or negative): use containerd's default lease expiration (24h), applied by client.WithLease when no lease opts are passed (see client/lease.go)
    • --timeout>0: use the specified duration
    • --timeout defaults to 1h for backward compatibility
  • Add --timeout CLI flag (default: 1h)
  • Add integration tests for --timeout=2h and --timeout=0s
  • Add a unit test covering --timeout flag parsing

Addressing the remaining review comment from #5108

--timeout=0 does not parse (time.ParseDuration("0") needs a unit). Help/docs advertise 0, but cobra Duration only accepts 0s.

time.ParseDuration special-cases a bare "0" as a valid duration without a unit (go/src/time/format.go, "Special case: if all that is left is "0", this is zero."). Since pflag's Duration flag delegates to time.ParseDuration, both --timeout=0 and --timeout=0s parse successfully.

The added unit test TestCommitTimeoutFlagParsing locks in this behavior for 0, 0s, 90m, 4h, and the 1h default, and the command reference now explicitly documents that both 0 and 0s are accepted.

Usage

nerdctl container commit mycontainer myimage:tag      # default 1h
nerdctl container commit --timeout=4h ...             # 4h timeout
nerdctl container commit --timeout=0 ...              # 24h (containerd default)

Fixes #5107
Supersedes #5108

@Luckydog691

Copy link
Copy Markdown
Contributor Author

The single failing check (in-host / nerdctl.gomodjail rootless) failed on TestRunRmTime (container_run_test.go:706: expected to have completed in 3s, took 12.9s), which is unrelated to this PR — it is the known flake tracked in #3988. The same job leg also fails on main at this PR's base commit (see the test workflow run for #5248). Going to close/reopen to retrigger CI.

@Luckydog691 Luckydog691 closed this Oct 8, 2026
@Luckydog691 Luckydog691 reopened this Oct 8, 2026
@AkihiroSuda AkihiroSuda added this to the v2.5.0 (tentative) milestone Oct 9, 2026

@ChengyuZhu6 ChengyuZhu6 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.

Nit: the commit metadata looks inconsistent — could you please fix it ?

The commit author is hejianshan hejianshan@bytedance.com, but the Signed-off-by trailer is Turbo Jia turbo@example.com, which looks like a placeholder address.

@AkihiroSuda AkihiroSuda modified the milestones: v2.5.0 (tentative), v2.4.2 Oct 9, 2026
hejianshan and others added 2 commits October 10, 2026 09:46
The hardcoded 1-hour lease expiration in nerdctl container commit can
cause failures for containers with large filesystem deltas (e.g., many
packages installed). When the diff.Compare operation exceeds 1 hour,
the containerd GC cleans up the expired lease and its ingest data,
resulting in 'context deadline exceeded' errors.

Changes:
- Add Timeout field to ContainerCommitOptions and commit.Opts
- Modify Commit() to use configurable lease expiration:
  * --timeout=0 (or negative): use containerd's default 24h lease
  * --timeout>0: use the specified duration
  * --timeout defaults to 1h for backward compatibility
- Add --timeout CLI flag (default: 1h)
- Add tests for --timeout flag

Usage:
  nerdctl container commit mycontainer myimage:tag        # default 1h
  nerdctl container commit --timeout=4h ...                # 4h timeout
  nerdctl container commit --timeout=0 ...                 # 24h (containerd default)

Signed-off-by: hejianshan <hejianshan@bytedance.com>
Address review feedback on containerd#5108 that "--timeout=0 does not parse".
time.ParseDuration special-cases a bare "0" as a valid duration
without a unit, so both "--timeout=0" and "--timeout=0s" are
accepted by the cobra Duration flag. Add a unit test locking in this
behavior and clarify the command reference accordingly.

Signed-off-by: liujiuyi <liujiuyi.ljy@alibaba-inc.com>
@Luckydog691
Luckydog691 force-pushed the fix/commit-timeout-flag branch from 3254832 to dae7288 Compare October 10, 2026 01:47
@Luckydog691

Copy link
Copy Markdown
Contributor Author

@ChengyuZhu6 Thanks for catching that. Fixed — the first commit keeps the original authorship (hejianshan <hejianshan@bytedance.com>) and the placeholder Signed-off-by: Turbo Jia <turbo@example.com> trailer has been corrected to Signed-off-by: hejianshan <hejianshan@bytedance.com>. No code changes (tree is identical).

@ChengyuZhu6 ChengyuZhu6 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

@ChengyuZhu6
ChengyuZhu6 merged commit ae7a794 into containerd:main Oct 10, 2026
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nerdctl container commit has a hardcoded 1-hour lease that causes failure for large container diffs?what about make it as flag

3 participants