Skip to content

Add WithFileBufferedOpener for file-backed daemon image buffering#2214

Merged
Subserial merged 9 commits intogoogle:mainfrom
twdamhore:fix/daemon-oom
Feb 24, 2026
Merged

Add WithFileBufferedOpener for file-backed daemon image buffering#2214
Subserial merged 9 commits intogoogle:mainfrom
twdamhore:fix/daemon-oom

Conversation

@twdamhore
Copy link
Copy Markdown
Contributor

@twdamhore twdamhore commented Feb 19, 2026 •
edited
Loading

Summary

  • Add WithFileBufferedOpener() option to daemon.Image() that buffers the docker save stream to a temp file instead of memory
  • Enables consumers (e.g. minikube) to avoid OOM kills when loading large images
  • Single docker save, ~32KB peak memory, file-backed reads

See: kubernetes/minikube#17785
See: kubernetes/minikube#21103

How it works

daemon.Image() currently has two modes:

  • Buffered (default): io.ReadAll into []byte — one save, but full image in RAM
  • Unbuffered: fresh ImageSave on every access — low memory but O(N) saves

This adds a third mode:

  • File-backed: io.Copy to temp file — one save, ~32KB peak memory
img, err := daemon.Image(ref, daemon.WithFileBufferedOpener())                                                                                                                                                                                   
// use img normally — all reads are file-backed, one docker save, no memory                                                                                                                                                                      
// temp file is automatically removed when imageOpener is garbage collected                                                                                                                                                                      
// (via runtime.AddCleanup)

Cleanup

The temp file is automatically cleaned up via runtime.AddCleanup on the imageOpener — no explicit close needed. If the GC doesn't run before process exit, the file sits in os.TempDir() until OS cleanup.

Benchmark (elasticsearch:9.0.3, 1.47GB)

Run with /usr/bin/time -v using the included bench_memory_test.go (-tags bench_memory):

/usr/bin/time -v go test -tags bench_memory -run TestBenchMemoryBacked -v ./pkg/v1/daemon/ -count=1                                                                                                                                              
/usr/bin/time -v go test -tags bench_memory -run TestBenchFileBacked -v ./pkg/v1/daemon/ -count=1
Mode Max RSS Wall time
Memory-backed 1,872 MB 7.20s
File-backed 172 MB 6.34s

10.9x memory reduction.

Test plan

  • Unit tests: file-buffered produces same image content as memory-buffered
  • Unit tests: temp file is created during access
  • Unit tests: save error and read error propagation
  • Existing buffered/unbuffered tests still pass
  • Memory benchmark with real Docker image (-tags bench_memory)

@google-cla
Copy link
Copy Markdown

google-cla bot commented Feb 19, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 19, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.17%. Comparing base (8b3c303) to head (79da0f3).
⚠️ Report is 66 commits behind head on main.

Files with missing lines Patch % Lines
pkg/v1/daemon/image.go 91.17% 2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (8b3c303) and HEAD (79da0f3). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (8b3c303) HEAD (79da0f3)
2 1
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #2214       +/-   ##
===========================================
- Coverage   71.67%   53.17%   -18.51%     
===========================================
  Files         123      164       +41     
  Lines        9935    10977     +1042     
===========================================
- Hits         7121     5837     -1284     
- Misses       2115     4433     +2318     
- Partials      699      707        +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@twdamhore
Suppress G703 security warning in image.go
5e36621 
Added a #nosec comment to suppress security warning for file creation.
@Subserial
Copy link
Copy Markdown
Contributor

Subserial commented Feb 19, 2026

Sorry about the gosec churn, we just updated our linter and it came with new features.

Subserial
Subserial reviewed Feb 19, 2026
View reviewed changes
@twdamhore twdamhore requested a review from Subserial February 19, 2026 23:56
Subserial
Subserial reviewed Feb 20, 2026
View reviewed changes
@twdamhore
Copy link
Copy Markdown
Contributor Author

twdamhore commented Feb 20, 2026

@Subserial thanks for the approval. Next it is saying 9 workflows awaiting approval. This is my first time contributing to this repository, should I just wait?

@Subserial
Copy link
Copy Markdown
Contributor

Subserial commented Feb 23, 2026

@Subserial thanks for the approval. Next it is saying 9 workflows awaiting approval. This is my first time contributing to this repository, should I just wait?

Yeah, I have to come back and approve the test run. Once the one goimports error is fixed I should be able to merge the PR.

@twdamhore
Copy link
Copy Markdown
Contributor Author

twdamhore commented Feb 23, 2026

@Subserial thanks for the approval. Next it is saying 9 workflows awaiting approval. This is my first time contributing to this repository, should I just wait?

Yeah, I have to come back and approve the test run. Once the one goimports error is fixed I should be able to merge the PR.

Thanks. I have removed the unused import.

@Subserial Subserial merged commit e971d63 into google:main Feb 24, 2026
17 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Feb 24, 2026
Merged
@dtrudg dtrudg mentioned this pull request Feb 25, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Subserial Subserial Subserial approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@twdamhore @codecov-commenter @Subserial