-
Notifications
You must be signed in to change notification settings - Fork 9
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
Don't copy local image #38
Conversation
03df41c
to
5009d5b
Compare
PTALA |
5009d5b
to
d0f6b2e
Compare
d0f6b2e
to
e9fc281
Compare
@praveenkumar PTALA |
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.
Couple of minor comments, but overall looks good to me.
Something that I was wondering during the review, should we create the overlay image in snc and ship it in the bundle? This has the advantage of not adding a qemu-img dependency on the libvirt machine driver (even if it's already there indirectly through libvirt). This would also mean we could try to use the overlay on macos too, where qemu-img is less likely to be present (but I don't know if hyperkit's qcow2 implementation supports that).
|
||
if u.Scheme == "file" || u.Scheme == "" { | ||
// For local image on Linux, we can avoid copying by making use of base image through qemu-img | ||
// Local disk image so we can avoid copying by making use of backing image feature of qcow2 |
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.
These 2 comments seem a bit redundant?
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 so?
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.
I don't see much difference in meaning/information provided by
For local image on Linux, we can avoid copying by making use of base image through qemu-img
and Local disk image so we can avoid copying by making use of backing image feature of qcow2
Both lines repeat "we are on linux, we have a local image, we can avoid the copy", and then you have 2 different ways of saying we are going to use an overlay instead of doing the copy.
e9fc281
to
611db9f
Compare
Would be worth investigating for sure but could we merge this now please? |
I've tested this, but I had some permissions issues when trying to create the VM and start it. I believe this is caused by |
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.
Could you add a full link in the commit log #37 ?
github unhelpfully shorten it, I mean have n explicit "Fixes https://github.com/ code-ready /machine-driver-libvirt/issues/37 " in the log. Easier to directly click on it when using git tools.
Would GH be able to pick it up and resolve the issue automatically? |
Yes |
On a machine with a rotating disk, this saves 2 minutes on startup time! (cold cache) |
@cfergeau I didn't try it out but it doesn't hurt to create as part of snc if that allow us the different format conversion without any issue. |
As discussed on slack, it turns out that with the hardlink approach, the source file in the cache directory will also change ownership to So this means that we should first get #20 in since that would mean |
If the source image is local, instead of copying it over, use qemu-img command to create an image with the source image as backing image instead. If `qemu-img` command is not available or the command fails for any reason, we revert back to copying the image. For crc, this reduces disk usage greatly (~ 9GB currently) and start up time by around 20 seconds on my very high-end system with an SSD. This adds an optional runtime dependency on `qemu-img` command. Fixes crc-org#37
611db9f
to
dcfc705
Compare
closing this since it is now fixed by #57 |
If the source image is local, instead of copying it over, use qemu-img command to create an image with the source image as backing image instead.
For crc, this reduces disk usage greatly (~ 9GB currently) and start up time by around 20 seconds on my very high-end system with an SSD.
This adds a runtime dependency on
qemu-img
command so we probably should check for that as part of CRC preflight checks.Fixes #37.