-
Notifications
You must be signed in to change notification settings - Fork 263
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
|
||
import org.ovirt.engine.core.bll.CommandActionState; | ||
import org.ovirt.engine.core.bll.DisableInPrepareMode; | ||
import org.ovirt.engine.core.bll.LockMessagesMatchUtil; | ||
import org.ovirt.engine.core.bll.NonTransactiveCommandAttribute; | ||
import org.ovirt.engine.core.bll.NonTransactiveCommandAttribute.CommandCompensationPhase; | ||
import org.ovirt.engine.core.bll.SerialChildCommandsExecutionCallback; | ||
|
@@ -44,30 +45,37 @@ | |
import org.ovirt.engine.core.common.businessentities.BiosType; | ||
import org.ovirt.engine.core.common.businessentities.OriginType; | ||
import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotStatus; | ||
import org.ovirt.engine.core.common.businessentities.StorageDomain; | ||
import org.ovirt.engine.core.common.businessentities.StorageDomainStatus; | ||
import org.ovirt.engine.core.common.businessentities.StorageDomainType; | ||
import org.ovirt.engine.core.common.businessentities.StoragePoolStatus; | ||
import org.ovirt.engine.core.common.businessentities.VDS; | ||
import org.ovirt.engine.core.common.businessentities.VDSStatus; | ||
import org.ovirt.engine.core.common.businessentities.VMStatus; | ||
import org.ovirt.engine.core.common.businessentities.VmDynamic; | ||
import org.ovirt.engine.core.common.businessentities.storage.DiskImage; | ||
import org.ovirt.engine.core.common.businessentities.storage.DiskVmElement; | ||
import org.ovirt.engine.core.common.businessentities.storage.StorageType; | ||
import org.ovirt.engine.core.common.businessentities.storage.VolumeFormat; | ||
import org.ovirt.engine.core.common.businessentities.storage.VolumeType; | ||
import org.ovirt.engine.core.common.errors.EngineException; | ||
import org.ovirt.engine.core.common.errors.EngineMessage; | ||
import org.ovirt.engine.core.common.job.Step; | ||
import org.ovirt.engine.core.common.job.StepEnum; | ||
import org.ovirt.engine.core.common.locks.LockingGroup; | ||
import org.ovirt.engine.core.common.queries.IdQueryParameters; | ||
import org.ovirt.engine.core.common.queries.QueryType; | ||
import org.ovirt.engine.core.common.utils.Pair; | ||
import org.ovirt.engine.core.common.utils.ValidationUtils; | ||
import org.ovirt.engine.core.common.vdscommands.VDSCommandType; | ||
import org.ovirt.engine.core.common.vdscommands.VdsAndVmIDVDSParametersBase; | ||
import org.ovirt.engine.core.compat.Guid; | ||
import org.ovirt.engine.core.dal.job.ExecutionMessageDirector; | ||
import org.ovirt.engine.core.dao.ClusterDao; | ||
import org.ovirt.engine.core.dao.DiskDao; | ||
import org.ovirt.engine.core.dao.StorageDomainDao; | ||
import org.ovirt.engine.core.dao.VdsDao; | ||
import org.ovirt.engine.core.dao.VmDao; | ||
|
||
@DisableInPrepareMode | ||
@NonTransactiveCommandAttribute(forceCompensation = true, compensationPhase = CommandCompensationPhase.END_COMMAND) | ||
|
@@ -92,11 +100,19 @@ public class ImportVmFromExternalProviderCommand<T extends ImportVmFromExternalP | |
@Inject | ||
private DiskDao diskDao; | ||
@Inject | ||
private VmDao vmDao; | ||
@Inject | ||
private StorageDomainDao storageDomainDao; | ||
@Inject | ||
private ImportUtils importUtils; | ||
@Inject | ||
@Typed(SerialChildCommandsExecutionCallback.class) | ||
private Instance<SerialChildCommandsExecutionCallback> callbackProvider; | ||
|
||
private StorageDomainType virtioIsoStorageDomainType; | ||
|
||
private StorageType virtioIsoStorageType; | ||
|
||
public ImportVmFromExternalProviderCommand(Guid cmdId) { | ||
super(cmdId); | ||
} | ||
|
@@ -189,6 +205,7 @@ 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 commentThe 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 commentThe 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. |
||
getActiveIsoDomainId() == null) { | ||
return failValidation(EngineMessage.ERROR_CANNOT_FIND_ISO_IMAGE_PATH); | ||
} | ||
|
@@ -428,6 +445,7 @@ private ConvertVmParameters buildConvertVmParameters() { | |
parameters.setStorageDomainId(getStorageDomainId()); | ||
parameters.setProxyHostId(getParameters().getProxyHostId()); | ||
parameters.setClusterId(getClusterId()); | ||
parameters.setVirtioIsoStorageDomainId(getParameters().getVirtioIsoStorageDomainId()); | ||
parameters.setVirtioIsoName(getParameters().getVirtioIsoName()); | ||
parameters.setNetworkInterfaces(getParameters().getVm().getInterfaces()); | ||
parameters.setParentCommand(getActionType()); | ||
|
@@ -468,6 +486,31 @@ public List<PermissionSubject> getPermissionCheckSubjects() { | |
return permissionList; | ||
} | ||
|
||
private void fillVirtioIsoStorageDomain() { | ||
if (getParameters().getVirtioIsoStorageDomainId() != null) { | ||
StorageDomain storageDomain = storageDomainDao.get(getParameters().getVirtioIsoStorageDomainId()); | ||
virtioIsoStorageDomainType = storageDomain.getStorageDomainType(); | ||
virtioIsoStorageType = storageDomain.getStorageType(); | ||
} else { | ||
virtioIsoStorageDomainType = StorageDomainType.ISO; | ||
virtioIsoStorageType = StorageType.NFS; | ||
} | ||
} | ||
|
||
private StorageDomainType getVirtioIsoStorageDomainType() { | ||
if (virtioIsoStorageDomainType == null) { | ||
fillVirtioIsoStorageDomain(); | ||
} | ||
return virtioIsoStorageDomainType; | ||
} | ||
|
||
private StorageType getVirtioIsoStorageType() { | ||
if (virtioIsoStorageType == null) { | ||
fillVirtioIsoStorageDomain(); | ||
} | ||
return virtioIsoStorageType; | ||
} | ||
|
||
protected Guid getActiveIsoDomainId() { | ||
return isoDomainListSynchronizer.findActiveISODomain(getStoragePoolId()); | ||
} | ||
|
@@ -575,4 +618,22 @@ private void deleteV2VJob() { | |
runVdsCommand(VDSCommandType.DeleteV2VJob, | ||
new VdsAndVmIDVDSParametersBase(getVdsId(), getVmId())); | ||
} | ||
|
||
@Override | ||
protected Map<String, Pair<String, String>> getExclusiveLocks() { | ||
var parentLocks = super.getExclusiveLocks(); | ||
if (getVirtioIsoStorageDomainType() == StorageDomainType.ISO || !getVirtioIsoStorageType().isBlockDomain()) { | ||
return parentLocks; | ||
} | ||
|
||
Map<String, Pair<String, String>> locks = new HashMap<>(); | ||
if (parentLocks != null) { | ||
locks.putAll(parentLocks); | ||
} | ||
locks.put(getParameters().getVirtioIsoName(), | ||
LockMessagesMatchUtil.makeLockingPair(LockingGroup.DISK, | ||
EngineMessage.ACTION_TYPE_FAILED_VIRTIO_ISO_IMAGE_BEING_USED)); | ||
return locks; | ||
} | ||
|
||
} |
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 forMeasureVolumeCommand
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 ofprepare/teardownImage()
, but because qemu cannot use a disk that was open for writing by another process. If a VM using the disk starts after startingMeasureVmCommand
, 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.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.