Skip to content
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

clone image before creating vm #51

Merged
merged 1 commit into from
Feb 18, 2025
Merged

clone image before creating vm #51

merged 1 commit into from
Feb 18, 2025

Conversation

lstocchi
Copy link
Collaborator

This PR changes the behavior of the noopImagePuller. As this puller does not actually pull any image as it is used when an image is already in the local system, it just clones the image in a dedicated folder so that the original image does not get alterated when started by macadam.

it resolves https://github.com/cfergeau/macadam/issues/41 and https://github.com/cfergeau/macadam/issues/42

@lstocchi lstocchi requested a review from cfergeau February 17, 2025 10:55
Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
but one comment regarding LocalPath

dirs, err := env.GetMachineDirs(vmType)
if err != nil {
return nil, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be computed locally in LocalPath, I think it's the only place where it's needed. Fine with me if you prefer to keep this code in NewNoopImagePuller

func (puller *NoopImagePuller) Download() error {
return nil
return copyFile(puller.sourceURI, puller.localPath.Path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work if LocalPath has been called before Download, this is currently the case but imo it's a bit unexpected.
LocalPath could be changed to return puller.localPath if it's already set, and to compute it if not. Then Download could always call LocalPath and be sure the path is correctly set.

bufferedWriter := bufio.NewWriter(out)
defer bufferedWriter.Flush()

_, err = io.Copy(bufferedWriter, in)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We eventually want to use CloneFile on macOS with a fallback to https://github.com/containers/podman/blob/main/pkg/machine/compression/sparse_file_writer.go, and qemu-img create with a backing file on linux, but this can be done as a follow up to this PR.

This PR changes the behavior of the noopImagePuller. As this puller does
not actually pull any image as it is used when an image is already in
the local system, it just clones the image in a dedicated folder so that
the original image does not get alterated when started by macadam.

Signed-off-by: lstocchi <[email protected]>
@cfergeau cfergeau merged commit 27d46e8 into crc-org:main Feb 18, 2025
5 of 6 checks passed
@lstocchi lstocchi deleted the i41 branch February 18, 2025 09:37
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.

cli: Use backing file for disk image
2 participants