-
Notifications
You must be signed in to change notification settings - Fork 261
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
core: Support virtio ISO from data domain in VM import #570
Conversation
return; | ||
} | ||
DiskImage isoImage = imagesHandler.getSnapshotLeaf(new Guid(getParameters().getVirtioIsoName())); | ||
imagesHandler.teardownImage(getStoragePoolId(), |
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.
and what if this iso is used concurrently by another VM on this host?
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.
Right. So, I can block this, like it is done in MeasureVolumeCommand
- it fails in validation, if the disk is used by a running VM.
But there is also a case when the same ISO is used by several ConvertVm
commands. I can use in-memory locks for this, for disks on data domains only.
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.
So it means we cannot attach the ISO if is used by an active VM? Does anything prevents using it by multiple active VMs not considering the import? Is it a limitation of prepare / tear down image on the Vdsm side, perhaps missing support for using and sharing read-only images (how do we use CDs if it is the case? or can we not use CDs from data domains)? Please explain in the commit message what we reasonably can do, cannot do and actually do 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.
Done.
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.
Another question is: what will happen, if some VM using the ISO image will be started after the ConvertVmCommand
is started? I don't see this solved for MeasureVolumeCommand
also. And cannot figure out a good way to prevent this.
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.
@mz-pdm In MeasureVolumeCommand
there was a different reason for blocking usage of the same disk by any active VMs. Not because of prepare/teardownImage()
, but because qemu cannot use a disk that was open for writing by another process. If a VM using the disk starts after starting MeasureVmCommand
, qemu will fail to start.
In our case the ISO disk will be opened for reading, but the limitation of prepare/teardownImage()
still exists. Although, it is quite unlikely that some VM will use the ISO with virtio drivers as a disk.
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.
There may be other ISOs that users might use in running VMs. If I understand you correctly, there is nothing preventing attempts to start multiple VMs using the same ISO, but it will fail in Vdsm on preparing the image.
If we cannot resolve it easily, we could perhaps do:
-
Providing a doc text explaining the limitations.
-
Does or could the user get a comprehensible error message on such a VM start failure?
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.
@mz-pdm I've talked with Benny about prepare/teardownImage()
. There are two important things:
teardownImage()
does not affect running VMs using the same disk. This situation is handled inside VDSM since 4.4.- It is true that several commands cannot use
prepare/teardownImage()
on the same disk simultaneously. But this is true for block storage only. It's not a problem for file storage.
I'll update the patch accordingly.
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.
Great, good news, thanks.
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.
Looks like a nice change but sharing the image requires clarification and the change should get additional review from another reviewer.
@@ -189,17 +202,39 @@ protected boolean validate() { | |||
|
|||
if (getVm().getOrigin() != OriginType.KVM && | |||
getParameters().getVirtioIsoName() != null && | |||
getVirtioIsoStorageDomainType() == StorageDomainType.ISO && |
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.
Cool, does this fix, together with the other changes here, https://bugzilla.redhat.com/2109021 ?
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. As I see, https://bugzilla.redhat.com/2109021 is the same bug as https://bugzilla.redhat.com/1721455.
Path to a virtio ISO image used in VM import was constructed assuming that the image is located in an ISO domain. To support ISO images located in a data domain we need to pass the storage domain ID to ConvertVmCommand and use prepareImage()/teardownImage() to obtain the path to the image on the host where VM conversion is running. Due to limitation of prepareImage()/teardownImage() on VDSM side, these calls cannot be nested. That's why the ISO image located in a block data domain can be used exclusively by a single instance of ConvertVmCommand. Change-Id: I835602a9019ecbcf4c63529a9d006bc43ee793be Bug-Url: https://bugzilla.redhat.com/1721455 Signed-off-by: Shmuel Melamud <smelamud@redhat.com>
Looks good to me. |
@mz-pdm So, can you approve? |
Path to a virtio ISO image used in VM import was constructed assuming
that the image is located in an ISO domain. To support ISO images
located in a data domain we need to pass the storage domain ID to
ConvertVmCommand and use prepareImage()/teardownImage() to obtain the
path to the image on the host where VM conversion is running.
Due to limitation of prepareImage()/teardownImage() on VDSM side, these
calls cannot be nested. That's why the ISO image located in a data
domain can be used exclusively by a single instance of ConvertVmCommand.
Usage of it by any running VM or by other ConvertVmCommand instances in
the same time is prohibited.
Change-Id: I835602a9019ecbcf4c63529a9d006bc43ee793be
Bug-Url: https://bugzilla.redhat.com/1721455
Signed-off-by: Shmuel Melamud smelamud@redhat.com