-
Notifications
You must be signed in to change notification settings - Fork 787
fix: Remove premature return after closing sparse diff file so Convert executes. #4421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| return fmt.Errorf("failed to create sparse diff disk %q: %w", diffDisk, err) | ||
| } | ||
| return diffDiskF.Close() | ||
| diffDiskF.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you test this PR?
The diffDisk created in L43 will be overwritten by a new disk converted from baseDisk in L57, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes diffDisk would get converted in L57 when the base image is ISO.
I have include the tests to verify the conditions when base image is ISO and non-ISO
Can you please clarify on what should be the desired behaviors in each test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the base disk is an ISO9660 image, the diff disk has to be a new empty disk.
("diff" is misnomer here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff disk when created as a sparse file in L49 is empty and has size 0 bytes.
On debugging, I find that after conversion the diffdisk size becomes equal to diskSizeInBytes.
To keep the diffDisk empty we can either:
- not convert the
diffDiskand leave it as a sparse file and eventually ignorediskImageFormatwhen the base disk is ISO. or, - convert the
diffDiskto the available formats while keepingdiskSizeInBytes=0in case of ISO base disk. The problem here arises when the disk is being converted toasifformat, the following error gets thrownspecified size 0 is smaller than the original image sizebecause oflima/pkg/imgutil/nativeimgutil/nativeimgutil.go
Lines 59 to 61 in 9dbf572
if size != nil && *size < srcImg.Size() { return fmt.Errorf("specified size %d is smaller than the original image size (%d) of %q", *size, srcImg.Size(), source) }
efb2779 to
ea17ac7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does not fix #4418:
$ limactl start ./hack/test-templates/alpine-iso-9p-writable.yaml --vm-type vz --set .vmOpts.vz.diskimageFormat=\"asif\" --set .mountType=null --tty=false --log-level fatal
$ limactl stop alpine-iso-9p-writable
INFO[0000] Sending SIGINT to hostagent process 62151
INFO[0000] Waiting for the host agent and the driver processes to shut down
INFO[0000] [hostagent] Received SIGINT, shutting down the host agent
INFO[0000] [hostagent] Shutting down the host agent
INFO[0000] [hostagent] Shutting down VZ
INFO[0003] [hostagent] [VZ] - vm state change: stopped
ERRO[0003] [hostagent] accept tcp 127.0.0.1:58662: use of closed network connection
INFO[0004] Waiting for the instance to shut down
INFO[0004] The instance alpine-iso-9p-writable has shut down
$ diskutil image info ~/.lima/alpine-iso-9p-writable/diffdisk
Image Format: RAW
Format Description: RAW read-write image
Size Info:
Empty Bytes: 0
Min Size Bytes: 512
Sector Count: 209715200
Total Bytes: 107374182400
Encryption Info:
Is Encrypted: 0
Partitions
0:
content-hint: GUID_partition_scheme
total-space: 107374182400
used-space: N/A
volume-name: N/A
1:
content-hint: Microsoft Basic Data
total-space: 167936
used-space: N/A
volume-name: N/A
2:
content-hint: EFI
total-space: 1474560
used-space: N/A
volume-name: NO NAME
3:
content-hint: Microsoft Basic Data
total-space: 81270784
used-space: N/A
volume-name: N/A
$ limactl --version
limactl version 2.0.2-25-gea17ac7c.m
updated console log to add missing |
e7cdb60 to
2951d80
Compare
|
norio-nomura
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that #4418 will be corrected.
I don't know if the call to MakeSparse needs to be left, but I don't think it's a problem.
pkg/driverutil/disk.go
Outdated
| } | ||
|
|
||
| err = diskUtil.MakeSparse(ctx, diffDiskF, 0) | ||
| err = diskUtil.MakeSparse(ctx, diffDiskF, diskSizeInBytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this MakeSparse still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, just an empty volume is enough. I have removed it in the amended commit.
…ty volume for diff disk so Convert executes. Ensure diffDisk is converted to the requested format instead of staying sparse. Signed-off-by: ashwat287 <[email protected]>
Add pkg/driverutil/disk_test.go covering: - Base image is ISO: EnsureDisk creates/keeps diffdisk and converts to asif and raw; base ISO remains unchanged (content hash and ISO signature). - Base image is non-ISO: EnsureDisk converts diffdisk to raw and asif. Signed-off-by: ashwat287 <[email protected]>
norio-nomura
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍🏻
Fixes #4418
Ensure diffDisk is converted to the requested format instead of staying sparse.