feat(disk): map a source code disk into vm /dev/vdX#95
feat(disk): map a source code disk into vm /dev/vdX#95ihexon merged 2 commits intooomol-lab:mainfrom
Conversation
WalkthroughAdds a source code disk feature: a new embedded tarball extractor at pkg/machine/disk/disk.go with ExtractSourceCodeDisk(ctx, targetDirPath, overwrite). Adds MachineConfig.GetSourceDiskPath() returning dataDir/source.ext4. Integrates creation of a source disk into the VM device list and calls extraction during startup (before starting the VM provider) in pkg/machine/shim/host.go. Updates an error message for StartVMProvider failure. Adds godoclint to disabled linters and a minor formatting change in cmd/init.go. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
ihexon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/machine/machine_common.go:
- Around line 142-143: The comment above the devices append is stale: it claims
"externalDisk **must** be at the end of the device" but the code appends
sourceDisk after externalDisk via the devices = append(devices, disk, rng,
netDevice, externalDisk, sourceDisk) expression; update the comment to reflect
the new ordering or re-evaluate ordering assumptions by confirming that placing
sourceDisk after externalDisk is safe for the guest/boot sequence. Locate the
append call (devices = append(... externalDisk, sourceDisk)) and either change
the comment to state the correct ordering requirement or reorder/remove
sourceDisk so externalDisk is truly last if required; add a brief note
referencing any guest/boot assumption checked.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/machine/disk/source.ext4.taris excluded by!**/*.tar
📒 Files selected for processing (5)
cmd/init.gopkg/machine/disk/disk.gopkg/machine/machine_common.gopkg/machine/shim/host.gopkg/machine/vmconfig/vmconfig.go
💤 Files with no reviewable changes (1)
- cmd/init.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/machine/shim/host.go (1)
pkg/machine/disk/disk.go (1)
ExtractSourceCodeDisk(21-43)
⏰ 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
🔇 Additional comments (6)
pkg/machine/machine_common.go (1)
129-132: LGTM!The
sourceDiskcreation follows the same pattern as other disk device initializations with proper error handling and a descriptive error message.pkg/machine/vmconfig/vmconfig.go (1)
294-296: LGTM!The method follows the established pattern of other path getters in this file and correctly joins the data directory with the source disk filename.
pkg/machine/shim/host.go (1)
106-109: LGTM!The source disk extraction is correctly placed in the startup sequence after network initialization and before VM provider start. The error handling follows the established pattern.
pkg/machine/disk/disk.go (3)
21-26: Existence check doesn't verify file integrity.The check at line 22 only verifies that
source.ext4exists, not whether it's complete or valid. If a previous extraction was interrupted, the file might be corrupt or incomplete. Consider verifying file size or checksum, or always overwrite if extraction was interrupted.One approach is to verify the expected file size:
info, err := os.Stat(filepath.Join(targetDirPath, "source.ext4")) if err == nil && !overwrite { // Optionally verify info.Size() matches expected size logrus.Infof("source code disk already exists, skip extraction") return nil }Alternatively, consider using a temporary file during extraction and renaming atomically on success.
28-40: LGTM!The extraction logic is well-implemented:
- Directory creation with appropriate permissions
- Good use of
-Sflag for sparse file handling (important for disk images)- Context-aware command execution for cancellation support
- Proper stderr capture for debugging extraction failures
18-19: Verify binary size impact from embedded disk image.Embedding
source.ext4.tardirectly in the binary will increase the executable size by the full size of the tarball. Ensure this is acceptable for your distribution strategy.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.golangci.yml:
- Line 56: Add proper godoc comments for the two exported functions so godoclint
can remain disabled or be re-enabled: write a one- or two-sentence comment
starting with the function name for ExtractSourceCodeDisk (in
pkg/machine/disk/disk.go) describing what it does, its inputs and returned
values/errors; do the same for GetSourceDiskPath (in
pkg/machine/vmconfig/vmconfig.go), describing its purpose, parameters and return
value; ensure comments begin with the exact exported identifier and mention any
important behavior or error conditions.
🧹 Nitpick comments (1)
pkg/machine/machine_common.go (1)
142-143: Device ordering comment could be more complete.The updated comment correctly states that
externalDiskmust be added afterbootableDisk, but doesn't mention the newsourceDiskplacement. Consider updating the comment to clarify the complete ordering requirements.📝 Suggested comment clarification
- // The externalDisk must be added to the devices queue after the bootableDisk + // Device order: bootableDisk first, then externalDisk and sourceDisk for data storage devices = append(devices, bootableDisk, rng, netDevice, externalDisk, sourceDisk)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.golangci.ymlpkg/machine/machine_common.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/machine/machine_common.go (1)
pkg/machine/vmconfig/vmconfig.go (1)
Bootable(159-162)
🔇 Additional comments (2)
pkg/machine/machine_common.go (2)
113-117: Good refactoring for clarity.Renaming
disktobootableDiskimproves readability and consistency with other disk variables likeexternalDiskandsourceDisk.
129-132: The source disk file existence is already guaranteed by the code structure. Inpkg/machine/shim/host.go, the extraction of the source disk (step 2) occurs beforeStartVMProvideris called (step 3), which eventually triggersCreateDynamicConfigureandSetupDevices. TheExtractSourceCodeDiskfunction either successfully extracts the file or returns early if it already exists, and any failure causes the operation to abort before reachingSetupDevices. The file path alignment is correct: extraction targetsmc.Dirs.DataDir/source.ext4, which matches the path used inSetupDevicesviamc.GetSourceDiskPath(). No additional file existence check is needed.Likely an incorrect or invalid review comment.
| - revive | ||
| - funcorder | ||
| - noinlineerr | ||
| - godoclint |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if new public APIs have godoc comments
echo "=== Checking ExtractSourceCodeDisk in pkg/machine/disk/disk.go ==="
rg -A 5 -B 1 'func ExtractSourceCodeDisk' pkg/machine/disk/disk.go || echo "File or function not found"
echo -e "\n=== Checking GetSourceDiskPath in pkg/machine/vmconfig/vmconfig.go ==="
rg -A 5 -B 1 'func.*GetSourceDiskPath' pkg/machine/vmconfig/vmconfig.go || echo "Function not found"
echo -e "\n=== Checking file structure of pkg/machine/disk/disk.go ==="
if [ -f "pkg/machine/disk/disk.go" ]; then
head -50 pkg/machine/disk/disk.go | tail -30
else
echo "File does not exist"
fi
echo -e "\n=== Checking file structure of pkg/machine/vmconfig/vmconfig.go ==="
if [ -f "pkg/machine/vmconfig/vmconfig.go" ]; then
head -50 pkg/machine/vmconfig/vmconfig.go | tail -30
else
echo "File does not exist"
fiRepository: oomol-lab/ovm-mac
Length of output: 2432
Add godoc comments to new public APIs before disabling godoclint.
The following public functions lack documentation:
ExtractSourceCodeDiskinpkg/machine/disk/disk.goGetSourceDiskPathinpkg/machine/vmconfig/vmconfig.go
Add godoc comments for these exported APIs or keep godoclint enabled to enforce documentation standards.
🤖 Prompt for AI Agents
In @.golangci.yml at line 56, Add proper godoc comments for the two exported
functions so godoclint can remain disabled or be re-enabled: write a one- or
two-sentence comment starting with the function name for ExtractSourceCodeDisk
(in pkg/machine/disk/disk.go) describing what it does, its inputs and returned
values/errors; do the same for GetSourceDiskPath (in
pkg/machine/vmconfig/vmconfig.go), describing its purpose, parameters and return
value; ensure comments begin with the exact exported identifier and mention any
important behavior or error conditions.
2 participants
tested in oomol studio: