optimize image copy on windows #217
Conversation
WalkthroughStreamlines the core copy path to a single streamed copy that creates the destination, and splits platform-specific behavior into build-tagged helpers: Windows uses Robocopy with a fallback, Darwin performs qcow2→RAW conversion when needed, and Unix delegates to the core stream copy and uses filepath-based extension detection. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant doCopyFile
participant copyFile
participant Robocopy
participant qcow2Reader
participant qcow2Converter
Caller->>doCopyFile: src *os.File, dest path
alt platform == windows
doCopyFile->>Robocopy: run robocopy(srcDir, destDir, file, args)
alt robocopy success (exit <8)
Robocopy-->>doCopyFile: success
doCopyFile->>doCopyFile: rename destDir/srcFile -> dest
else robocopy unavailable/fails
doCopyFile->>copyFile: fallback stream copy
end
else platform == darwin
doCopyFile->>qcow2Reader: open src with qcow2reader
alt src is raw
qcow2Reader->>copyFile: stream copy raw
else src is qcow2
qcow2Reader->>qcow2Converter: convert to RAW -> dest
end
else (unix/linux)
doCopyFile->>copyFile: stream copy (uses filepath.Ext for extension)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/imagepullers/noop.go (1)
122-159: Excellent Windows optimization implementation with minor robustness suggestions.The robocopy implementation is well-designed with proper fallback, correct exit code handling, and appropriate flags for large file copying. The ~40% performance improvement mentioned in the PR objectives should be achieved with this approach.
Consider these minor robustness improvements:
func copyFileWin(srcF *os.File, dest string) error { binary, err := exec.LookPath("robocopy") if err != nil { logrus.Debugf("warning: failed to get robocopy binary: %v. Falling back to default file copy between %s and %s\n", err, srcF.Name(), dest) return copyFile(srcF, dest) } srcDir, srcFile := filepath.Split(srcF.Name()) + if srcFile == "" { + return fmt.Errorf("invalid source file path: %s", srcF.Name()) + } destDir, _ := filepath.Split(dest) cmd := exec.Command(binary, "/J", "/MT", "/R:0", srcDir, destDir, srcFile) if logrus.IsLevelEnabled(logrus.DebugLevel) { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr } err = cmd.Run() if err != nil { // robocopy does not use classic exit codes like linux commands, so we need to check for specific errors // Only exit code 8 is considered an error, all other exit codes are considered successful with exceptions // see https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/robocopy if exitErr, ok := err.(*exec.ExitError); ok { exitCode := exitErr.ExitCode() if exitCode >= 8 { return fmt.Errorf("failed to run robocopy: %w", err) } } else { return fmt.Errorf("failed to run robocopy: %w", err) } } + tempPath := filepath.Join(destDir, srcFile) err = os.Rename(filepath.Join(destDir, srcFile), dest) if err != nil { + // Clean up temporary file on rename failure + os.Remove(tempPath) return fmt.Errorf("failed to rename file: %w", err) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/imagepullers/noop.go(3 hunks)
🔇 Additional comments (5)
pkg/imagepullers/noop.go (5)
8-8: LGTM! Import additions support the new functionality.The added imports (
os/execfor robocopy execution andlogrusfor logging) are appropriate and necessary for the Windows optimization feature.Also applies to: 11-12
95-95: LGTM! Consistent signature updates across copy functions.The updated function calls consistently pass destination paths as strings rather than file handles, enabling specialized copy implementations (particularly the Windows robocopy optimization) while maintaining a uniform interface.
Also applies to: 97-97, 99-99
103-103: LGTM! Signature update aligns with refactoring pattern.The function signature change from
*os.Filetostringfor the destination parameter is consistent with the overall refactoring, allowing downstream functions to handle file creation internally.
161-172: LGTM! Consistent refactoring with proper file handling.The function correctly implements the signature change pattern, creating the destination file internally with appropriate error handling and cleanup via defer.
175-187: LGTM! Proper refactoring maintains performance characteristics.The function correctly implements the signature change pattern with internal file creation, proper cleanup, and maintains the buffered writing approach for good performance.
pkg/imagepullers/noop.go
Outdated
| } | ||
| } | ||
|
|
||
| err = os.Rename(filepath.Join(destDir, srcFile), dest) |
There was a problem hiding this comment.
What happens if there was already an unrelated file named destDir/srcFile in the destination directory?
There was a problem hiding this comment.
The existing file will be overwritten
There was a problem hiding this comment.
Are you sure about this? The exit code doc seems to say otherwise?
0- No files were copied. No failure was encountered. No files were mismatched. The files already exist in the destination directory; therefore, the copy operation was skipped.
There was a problem hiding this comment.
I'm gonna try but I have based my answer on the description of rename
Rename renames (moves) oldpath to newpath. If newpath already exists and is not a directory, Rename replaces it.
There was a problem hiding this comment.
Ah oops, I misplaced my comment and was not explicit enough, I was asking about robocopy behaviour when the destDir/srcFile already exists.
There was a problem hiding this comment.
by reading around it seems that on win it behaves differently and it fails. I'll fix it
There was a problem hiding this comment.
Updated. Added the flag /IS which overwrite if the file exists. I also verified rename works as expected on Windows, and it does. I added a remove command as suggested by the AI btw
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
pkg/imagepullers/noop.go (1)
44-50: Cross-OS extension differences: confirm expectationsimageExtension is now OS-specific. On Unix it preserves multi-extensions (see my comment in noop_unix.go), on Windows it has WSL-specific rules. Please confirm downstream code does not rely on previous extension behavior for imports/detection.
🧹 Nitpick comments (5)
pkg/imagepullers/noop_darwin.go (2)
24-26: Add context to image open error for easier diagnosisIncluding the source path makes troubleshooting much easier.
- if err != nil { - return err - } + if err != nil { + return fmt.Errorf("open source image %q: %w", src.Name(), err) + }
39-51: Make conversion atomic and clean up on failuresWriting directly to dest can leave a partially written file on error. Write to a temp file in the destination directory and rename on success to ensure atomicity and automatic cleanup.
@@ -import ( - "fmt" - "os" +import ( + "fmt" + "os" + "path/filepath" @@ func convertToRaw(srcImg image.Image, dest string) error { - destF, err := os.Create(dest) + // Write to a temp file and atomically move it into place on success. + destDir := filepath.Dir(dest) + tmp, err := os.CreateTemp(destDir, filepath.Base(dest)+".tmp-*") if err != nil { return err } - defer destF.Close() + tmpPath := tmp.Name() + // Ensure cleanup on failure paths. + defer func() { + tmp.Close() + // Ignore errors; tmpPath may have been renamed already. + _ = os.Remove(tmpPath) + }() if err := srcImg.Readable(); err != nil { return fmt.Errorf("source image is not readable: %w", err) } - return convert.Convert(destF, srcImg, convert.Options{}) + if err := convert.Convert(tmp, srcImg, convert.Options{}); err != nil { + return err + } + if err := tmp.Close(); err != nil { + return err + } + // Move the completed file into place. + return os.Rename(tmpPath, dest) }Also applies to: 5-16
pkg/imagepullers/noop_windows.go (2)
16-27: Handle multi-part extensions for WSL and general casesCurrent logic returns ".tar.gz" unconditionally for WSL (unless ".wsl"). This can mislabel ".tar", ".tar.xz", etc. Suggest recognizing common multi-extensions, defaulting to ".tar.gz" only when unknown.
import ( "errors" "fmt" + "io" "os" "os/exec" "path/filepath" + "strings" "github.com/containers/podman/v5/pkg/machine/define" "github.com/sirupsen/logrus" ) @@ func imageExtension(vmType define.VMType, sourceURI string) string { - switch vmType { - case define.WSLVirt: - ext := filepath.Ext(sourceURI) - if ext == ".wsl" { - return ".wsl" - } - return ".tar.gz" - default: - return filepath.Ext(sourceURI) - } + if vmType == define.WSLVirt { + lower := strings.ToLower(sourceURI) + switch { + case strings.HasSuffix(lower, ".wsl"): + return ".wsl" + case strings.HasSuffix(lower, ".tar.gz"), + strings.HasSuffix(lower, ".tar.xz"), + strings.HasSuffix(lower, ".tar.zst"), + strings.HasSuffix(lower, ".tar.bz2"), + strings.HasSuffix(lower, ".tar"): + i := strings.LastIndex(lower, ".tar") + return lower[i:] + default: + return ".tar.gz" + } + } + lower := strings.ToLower(sourceURI) + switch { + case strings.HasSuffix(lower, ".tar.gz"), + strings.HasSuffix(lower, ".tar.xz"), + strings.HasSuffix(lower, ".tar.zst"), + strings.HasSuffix(lower, ".tar.bz2"), + strings.HasSuffix(lower, ".tar"): + i := strings.LastIndex(lower, ".tar") + return lower[i:] + default: + return filepath.Ext(sourceURI) + } }Would you prefer limiting multi-extension handling only to WSL paths to minimize changes?
Also applies to: 5-14
32-33: Nit: logging tone and newlineThe message is logged at Debug level but says "warning:" and includes an extra newline.
- logrus.Debugf("warning: failed to get robocopy binary: %v. Falling back to default file copy between %s and %s\n", err, src.Name(), dest) + logrus.Debugf("robocopy not found (%v); falling back to default file copy between %s and %s", err, src.Name(), dest)pkg/imagepullers/noop.go (1)
72-84: Avoid double buffering; write directly to dest (optionally fsync)bufio.NewWriter plus io.Copy adds redundant buffering without benefit here. Writing directly to the file is simpler and often faster. Optionally fsync to reduce risk of partial data after a crash.
-import ( - "bufio" - "fmt" - "io" - "os" +import ( + "fmt" + "io" + "os" @@ -func copyFile(src *os.File, dest string) error { - destF, err := os.Create(dest) +func copyFile(src *os.File, dest string) error { + destF, err := os.Create(dest) if err != nil { return err } - defer destF.Close() + defer destF.Close() - bufferedWriter := bufio.NewWriter(destF) - defer bufferedWriter.Flush() - - _, err = io.Copy(bufferedWriter, src) - return err + if _, err = io.Copy(destF, src); err != nil { + return err + } + // Optional, uncomment if durability is desired over speed: + // return destF.Sync() + return nil }Also applies to: 3-12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
pkg/imagepullers/noop.go(1 hunks)pkg/imagepullers/noop_darwin.go(1 hunks)pkg/imagepullers/noop_unix.go(1 hunks)pkg/imagepullers/noop_windows.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/imagepullers/noop_darwin.go (1)
vendor/github.com/lima-vm/go-qcow2reader/convert/convert.go (1)
Convert(139-225)
pkg/imagepullers/noop.go (3)
vendor/github.com/lima-vm/go-qcow2reader/image/raw/raw.go (1)
Open(55-57)vendor/github.com/lima-vm/go-qcow2reader/qcow2reader.go (1)
Open(30-42)vendor/github.com/lima-vm/go-qcow2reader/image/qcow2/qcow2.go (1)
Open(556-644)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (oldstable)
🔇 Additional comments (4)
pkg/imagepullers/noop_darwin.go (1)
18-20: LGTM: extension determination matches VM image format on macOSUsing VMType.ImageFormat().Kind() keeps the filename aligned with the target runtime format.
pkg/imagepullers/noop_unix.go (1)
16-18: LGTM: straightforward delegation to copy pathThis keeps Unix simple and leverages the shared implementation.
pkg/imagepullers/noop_windows.go (1)
45-56: LGTM: Robocopy exit code handlingTreating 0–7 as success and >=8 as failure matches Microsoft’s documented semantics. Fallback path for non-ExitError is correct.
pkg/imagepullers/noop.go (1)
63-70: LGTM: centralizing the copy path and deferring to OS-specific helpersOpening the source once and delegating to doCopyFile aligns with the new design and keeps the flow clean.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/imagepullers/noop.go (1)
63-67: Wrap source open error with context for better diagnosabilityReturning the raw error here makes troubleshooting harder. Add context with the source path.
- src, err := os.Open(puller.sourceURI) - if err != nil { - return err - } + src, err := os.Open(puller.sourceURI) + if err != nil { + return fmt.Errorf("open source image %q: %w", puller.sourceURI, err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
pkg/imagepullers/noop.go(1 hunks)pkg/imagepullers/noop_darwin.go(1 hunks)pkg/imagepullers/noop_unix.go(1 hunks)pkg/imagepullers/noop_windows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/imagepullers/noop_darwin.go
- pkg/imagepullers/noop_windows.go
- pkg/imagepullers/noop_unix.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/imagepullers/noop.go (9)
vendor/github.com/lima-vm/go-qcow2reader/image/parallels/parallels.go (1)
Open(13-18)vendor/github.com/lima-vm/go-qcow2reader/image/raw/raw.go (1)
Open(55-57)vendor/github.com/lima-vm/go-qcow2reader/image/vdi/vdi.go (1)
Open(15-25)vendor/github.com/lima-vm/go-qcow2reader/image/vhdx/vhdx.go (1)
Open(13-15)vendor/github.com/lima-vm/go-qcow2reader/image/vpc/vpc.go (1)
Open(13-15)vendor/github.com/lima-vm/go-qcow2reader/image/vmdk/vmdk.go (1)
Open(13-19)vendor/github.com/lima-vm/go-qcow2reader/qcow2reader.go (1)
Open(30-42)vendor/github.com/lima-vm/go-qcow2reader/image/qcow2/qcow2.go (1)
Open(556-644)vendor/github.com/containers/storage/pkg/pools/pools.go (1)
Copy(62-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (oldstable)
this commit adds an optimization of the image copy process on Windows. It leverages robocopy, when available, which is a file replication tool built into Windows. By using it we reduce the time to copy of ~40%. Robocopy is launched with flags: - /J: Copies using unbuffered I/O (recommended for large files) - /MT: Creates multi-threaded copies with n threads. Default value of n is 8 - /R:<n> Specifies the number of retries on failed copies. 0 in this patch - /IS Includes Same files, which forces an overwrite even if the destination file appears identical to the source. Robocopy also uses specific exit code numbers which does not follow classic values. E.g. exit code 1 means all copies has been completed successfully. All exit codes are listed at https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/robocopy#exit-return-codes If robocopy is not found on the Win system, it fallback to the old copyFile function. Signed-off-by: lstocchi <lstocchi@redhat.com>
lstocchi
force-pushed
the
i69
branch
2 times, most recently
from
df82944 to
9fa6017
Compare
noop contains funcs that are specific to different OS. This commit creates OS specific noop file that implements their logic. This cleans the noop file of all switches/if. This is a simple refactor, no logic is changed Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/imagepullers/noop_windows.go (2)
36-37: Resolve relative src paths before invoking robocopyWhen src.Name() is relative, filepath.Split returns an empty directory, which breaks robocopy’s expected “source directory” argument.
Apply this diff to normalize to an absolute path before splitting:
-srcDir, srcFile := filepath.Split(src.Name()) +srcPath := src.Name() +if !filepath.IsAbs(srcPath) { + if abs, absErr := filepath.Abs(srcPath); absErr == nil { + srcPath = abs + } +} +srcDir, srcFile := filepath.Split(srcPath)
5-14: Fix build: import io for io.DiscardThe Windows build fails because io.Discard is used without importing io.
Apply this diff to fix the import:
import ( "errors" "fmt" + "io" "os" "os/exec" "path/filepath" "github.com/containers/podman/v5/pkg/machine/define" "github.com/sirupsen/logrus" )pkg/imagepullers/noop.go (1)
72-84: Avoid bufio.Writer here; check Sync/Close errors and clean up partial filesio.Copy already buffers internally. Flushing errors are currently ignored, and durability isn’t ensured.
Apply this diff to simplify and harden copy semantics:
-func copyFile(src *os.File, dest string) error { - destF, err := os.Create(dest) - if err != nil { - return err - } - defer destF.Close() - - bufferedWriter := bufio.NewWriter(destF) - defer bufferedWriter.Flush() - - _, err = io.Copy(bufferedWriter, src) - return err -} +func copyFile(src *os.File, dest string) error { + destF, err := os.Create(dest) + if err != nil { + return err + } + + // Copy with io.Copy (has its own internal buffer). + _, err = io.Copy(destF, src) + // Ensure bytes hit disk before returning success. + if syncErr := destF.Sync(); err == nil && syncErr != nil { + err = syncErr + } + // Propagate close error if no prior error occurred. + if closeErr := destF.Close(); err == nil && closeErr != nil { + err = closeErr + } + // Best-effort cleanup of partial file on error. + if err != nil { + _ = os.Remove(dest) + } + return err +}
🧹 Nitpick comments (3)
pkg/imagepullers/noop_windows.go (2)
56-58: Clarify robocopy exit-code comment (0–7 success; >= 8 failure)The code handles exit codes correctly, but the comment is misleading. Update it for accuracy.
- // robocopy does not use classic exit codes like linux commands, so we need to check for specific errors - // Only exit code 8 is considered an error, all other exit codes are considered successful with exceptions - // see https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/robocopy + // robocopy does not use classic exit codes like Linux commands. + // Per Microsoft docs, exit codes 0–7 indicate success or non-fatal conditions; codes >= 8 indicate failure. + // See https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/robocopy#return-codes
32-33: Nit: remove trailing newline from Debugflogrus handles line endings; embedding “\n” in the format string is unnecessary.
- logrus.Debugf("warning: failed to get robocopy binary: %v. Falling back to default file copy between %s and %s\n", err, src.Name(), dest) + logrus.Debugf("warning: failed to get robocopy binary: %v. Falling back to default file copy between %s and %s", err, src.Name(), dest)pkg/imagepullers/noop.go (1)
3-12: Remove unused bufio import after refactorIf you apply the copyFile refactor, bufio becomes unused.
import ( - "bufio" "fmt" "io" "os" "github.com/containers/podman/v5/pkg/machine/define" "github.com/containers/podman/v5/pkg/machine/env" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
pkg/imagepullers/noop.go(1 hunks)pkg/imagepullers/noop_darwin.go(1 hunks)pkg/imagepullers/noop_unix.go(1 hunks)pkg/imagepullers/noop_windows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/imagepullers/noop_unix.go
- pkg/imagepullers/noop_darwin.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T13:21:37.636Z
Learnt from: lstocchi
PR: crc-org/macadam#217
File: pkg/imagepullers/noop_windows.go:61-64
Timestamp: 2025-08-18T13:21:37.636Z
Learning: os.Rename works correctly on Windows even when the destination file exists, based on user testing in Windows environments.
Applied to files:
pkg/imagepullers/noop_windows.go
🧬 Code Graph Analysis (1)
pkg/imagepullers/noop.go (3)
vendor/github.com/lima-vm/go-qcow2reader/image/raw/raw.go (1)
Open(55-57)vendor/github.com/lima-vm/go-qcow2reader/qcow2reader.go (1)
Open(30-42)vendor/github.com/lima-vm/go-qcow2reader/image/qcow2/qcow2.go (1)
Open(556-644)
🪛 GitHub Check: build (stable)
pkg/imagepullers/noop_windows.go
[failure] 51-51:
undefined: io
[failure] 50-50:
undefined: io
🪛 GitHub Check: build (oldstable)
pkg/imagepullers/noop_windows.go
[failure] 51-51:
undefined: io
[failure] 50-50:
undefined: io
🪛 GitHub Actions: Build
pkg/imagepullers/noop_windows.go
[error] 50-50: Go build failed for Windows target: undefined: io in pkg/imagepullers/noop_windows.go:50:16. Step: 'go build' for Windows (part of 'make cross').
🔇 Additional comments (3)
pkg/imagepullers/noop_windows.go (2)
45-52: Silencing robocopy output by default looks goodGood practice: default to discarding stdout/stderr and only surface output when debug logging is enabled.
70-77: Overwrite semantics + final rename: LGTMPre-removing the destination and then renaming the copied file in-place provides predictable overwrite behavior and avoids cross-volume moves. Given the earlier validation that os.Rename behaves as expected on Windows, this is solid.
pkg/imagepullers/noop.go (1)
63-70: File lifecycle handling is correctOpening the source here and deferring Close() at the caller while delegating the copy to doCopyFile aligns ownership and avoids premature closes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/imagepullers/noop.go (1)
72-84: Do not ignore flush errors; remove unnecessary buffering; fsync and clean up partial files on errorBuffered writer is unnecessary (io.Copy already buffers) and the deferred Flush error is ignored. Capture Sync/Close errors and remove partial files on failure. Also drop the now-unused bufio import.
Apply this diff:
-func copyFile(src *os.File, dest string) error { - destF, err := os.Create(dest) - if err != nil { - return err - } - defer destF.Close() - - bufferedWriter := bufio.NewWriter(destF) - defer bufferedWriter.Flush() - - _, err = io.Copy(bufferedWriter, src) - return err -} +func copyFile(src *os.File, dest string) error { + destF, err := os.Create(dest) + if err != nil { + return err + } + + // Copy with io.Copy (has its own internal buffer). + _, err = io.Copy(destF, src) + // Ensure data hits disk before reporting success. + if syncErr := destF.Sync(); err == nil && syncErr != nil { + err = syncErr + } + // Close and propagate any close error if no prior error. + if closeErr := destF.Close(); err == nil && closeErr != nil { + err = closeErr + } + // Best-effort cleanup of partial file on error. + if err != nil { + _ = os.Remove(dest) + } + return err +}Follow-up:
- Remove "bufio" from the import list in this file.
- Optional performance tweak: use github.com/containers/storage/pkg/pools.Copy(destF, src) for fewer allocations.
🧹 Nitpick comments (1)
pkg/imagepullers/noop.go (1)
69-70: Preserve error context from doCopyFileWrap the error so callers/logs know where the copy was headed.
Apply this diff:
- return doCopyFile(src, localPath.Path) + if err := doCopyFile(src, localPath.Path); err != nil { + return fmt.Errorf("copy image to %q: %w", localPath.Path, err) + } + return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
pkg/imagepullers/noop.go(1 hunks)pkg/imagepullers/noop_darwin.go(1 hunks)pkg/imagepullers/noop_unix.go(1 hunks)pkg/imagepullers/noop_windows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/imagepullers/noop_windows.go
- pkg/imagepullers/noop_darwin.go
- pkg/imagepullers/noop_unix.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/imagepullers/noop.go (6)
vendor/github.com/lima-vm/go-qcow2reader/image/raw/raw.go (1)
Open(55-57)vendor/github.com/lima-vm/go-qcow2reader/image/vdi/vdi.go (1)
Open(15-25)vendor/github.com/lima-vm/go-qcow2reader/image/vmdk/vmdk.go (1)
Open(13-19)vendor/github.com/lima-vm/go-qcow2reader/image/vpc/vpc.go (1)
Open(13-15)vendor/github.com/lima-vm/go-qcow2reader/qcow2reader.go (1)
Open(30-42)vendor/github.com/lima-vm/go-qcow2reader/image/qcow2/qcow2.go (1)
Open(556-644)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e (ubuntu-24.04)
- GitHub Check: test (windows-latest)
- GitHub Check: lint
🔇 Additional comments (1)
pkg/imagepullers/noop.go (1)
63-67: Wrap os.Open error with context; ownership boundary remains clearIn pkg/imagepullers/noop.go (lines 63–67), update the error return:
- src, err := os.Open(puller.sourceURI) - if err != nil { - return err - } + src, err := os.Open(puller.sourceURI) + if err != nil { + return fmt.Errorf("open source image %q: %w", puller.sourceURI, err) + }• Verified that none of the platform-specific doCopyFile implementations (noop_windows.go, noop_unix.go, noop_darwin.go) invoke src.Close(), so the deferred close remains the sole owner of the file.
this commit adds an optimization of the image copy process on Windows. It leverages robocopy, when available, which is a file replication tool built into Windows. By using it we reduce the time to copy of ~40%.
Robocopy is launched with flags:
Robocopy also uses specific exit code numbers which does not follow classic values. E.g. exit code 1 means all copies has been completed successfully. All exit codes are listed at https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/robocopy#exit-return-codes
If robocopy is not found on the Win system, it fallback to the old copyFile function.
it resolves #69
Summary by CodeRabbit
Bug Fixes
New Features
Refactor