Skip to content

Commit

Permalink
core: add a destination Storage Domain ID support in "MoveOrCopyImage…
Browse files Browse the repository at this point in the history
…GroupParameters"

Add a clean and naming-consistent destination Storage Domain ID support in
the "MoveOrCopyImageGroupParameters" class.
Actually hiding the usage of the data member from "StorageDomainParametersBase"
superclass and wrapping it's getter & setter by the new ones.

1. Fix naming of the getter from "getTargetStorageDomainId()" to a more consistent
"getDestDomainId()" and move it from "LiveMigrateDiskParameters" class
to its parent's "MoveOrCopyImageGroupParameters".
2. Add a setter "setDestDomainId()" in "MoveOrCopyImageGroupParameters" class.
3. Refactoring code over multiple classes to use the above accessor & mutator.
4. Better and consistent names for "MoveOrCopyImageGroupParameters" Constructors.

Background:
Commands performing actions on 2 Storage Domains (source & destination)
use "MoveOrCopyImageGroupParameters" or its subclasses.
Until "MoveOrCopyImageGroupParameters" there was only 1 Storage Domain
used - the one that the action was performed on.
The support for source Storage Domain ID was added in
"MoveOrCopyImageGroupParameters", while the already existing
(in "StorageDomainParametersBase" superclass) Storage Domain
ID was implicitly used as the destination Storage Domain ID.
Some additional support to easier reference source & destination Storage Domains
was added in "LiveMigrateDiskParameters".
Still there were a few issues - a missing clear destination
Storage Domain ID setter and also inconsistent naming between the classes.

Refactoring the code to add the full and naming-consistent source & destination
Storage Domains getters and setters in the "MoveOrCopyImageGroupParameters" class,
thus those will be inherited by its subclasses (i.e., "LiveMigrateDiskParameters").

Signed-off-by: Pavel Bar <pbar@redhat.com>
  • Loading branch information
barpavel committed Sep 12, 2022
1 parent 194b1d6 commit 63ca05c
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ protected boolean noCopyBetweenLocalAndManagedBlockStorages() {

protected boolean isSourceAndDestTheSame() {
if (isMoveOperation()
&& getParameters().getSourceDomainId().equals(getParameters().getStorageDomainId())) {
&& getParameters().getSourceDomainId().equals(getParameters().getDestDomainId())) {
return failValidation(EngineMessage.ACTION_TYPE_FAILED_SOURCE_AND_TARGET_SAME);
}
return true;
Expand Down Expand Up @@ -362,7 +362,7 @@ protected boolean checkTemplateDiskExistAndValidInDestStorageDomain() {
if (isMoveOperation()
&& !Guid.Empty.equals(getImage().getImageTemplateId())) {
DiskImage templateImage = diskImageDao.get(getImage().getImageTemplateId());
if (!templateImage.getStorageIds().contains(getParameters().getStorageDomainId())) {
if (!templateImage.getStorageIds().contains(getParameters().getDestDomainId())) {
return failValidation(EngineMessage.ACTION_TYPE_FAILED_TEMPLATE_NOT_FOUND_ON_DESTINATION_DOMAIN);
}
if (templateImage.getImageStatus() != ImageStatus.OK) {
Expand Down Expand Up @@ -403,7 +403,7 @@ protected void executeCommand() {
private void addDiskMapping() {
executeInNewTransaction(() -> {
addStorageDomainMapForCopiedTemplateDisk();
unregisteredDisksDao.removeUnregisteredDisk(getImage().getId(), getParameters().getStorageDomainId());
unregisteredDisksDao.removeUnregisteredDisk(getImage().getId(), getParameters().getDestDomainId());
incrementDbGenerationForRelatedEntities();
return null;
});
Expand All @@ -419,7 +419,7 @@ protected boolean isUnregisteredDiskExistsForCopyTemplate() {
if (isTemplate() && isCopyOperation()) {
List<UnregisteredDisk> unregisteredDisks =
unregisteredDisksDao.getByDiskIdAndStorageDomainId(getImage().getId(),
getParameters().getStorageDomainId());
getParameters().getDestDomainId());
if (!unregisteredDisks.isEmpty()) {
return true;
}
Expand All @@ -430,7 +430,7 @@ protected boolean isUnregisteredDiskExistsForCopyTemplate() {
private void addStorageDomainMapForCopiedTemplateDisk() {
imageStorageDomainMapDao.save
(new ImageStorageDomainMap(getParameters().getImageId(),
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getQuotaId(),
getImage().getDiskProfileId()));
}
Expand Down Expand Up @@ -503,7 +503,7 @@ public List<PermissionSubject> getPermissionCheckSubjects() {
DiskImage image = getImage();
Guid diskId = image == null ? Guid.Empty : image.getId();
cachedPermsList.add(new PermissionSubject(diskId, VdcObjectType.Disk, ActionGroup.CONFIGURE_DISK_STORAGE));
cachedPermsList.add(new PermissionSubject(getParameters().getStorageDomainId(),
cachedPermsList.add(new PermissionSubject(getParameters().getDestDomainId(),
VdcObjectType.Storage, ActionGroup.CREATE_DISK));
}
return cachedPermsList;
Expand Down Expand Up @@ -565,7 +565,7 @@ private void prepareCopyNotTemplate(MoveOrCopyImageGroupParameters parameters) {

image.setDiskAlias(getDiskAlias());
image.setStorageIds(new ArrayList<>());
image.getStorageIds().add(getParameters().getStorageDomainId());
image.getStorageIds().add(getParameters().getDestDomainId());
image.setQuotaId(getParameters().getQuotaId());
image.setDiskProfileId(getParameters().getDiskProfileId());
image.setImageStatus(ImageStatus.LOCKED);
Expand Down Expand Up @@ -640,7 +640,7 @@ protected boolean setAndValidateDiskProfiles() {

getImage().setDiskProfileId(getParameters().getDiskProfileId());
return validate(diskProfileHelper.setAndValidateDiskProfiles(Collections.singletonMap(getImage(),
getParameters().getStorageDomainId()), getCurrentUser()));
getParameters().getDestDomainId()), getCurrentUser()));
}

public boolean setAndValidateQuota() {
Expand All @@ -658,15 +658,15 @@ public boolean setAndValidateQuota() {

QuotaValidator validator = createQuotaValidator(getDestinationQuotaId());
return validate(validator.isValid()) &&
validate(validator.isDefinedForStorageDomain(getParameters().getStorageDomainId()));
validate(validator.isDefinedForStorageDomain(getParameters().getDestDomainId()));
}

protected boolean validatePassDiscardSupportedForDestinationStorageDomain() {
if (isMoveOperation() ||
isCopyOperation() && isTemplate()) {
MultipleDiskVmElementValidator multipleDiskVmElementValidator = createMultipleDiskVmElementValidator();
return validate(multipleDiskVmElementValidator.isPassDiscardSupportedForDestSd(
getParameters().getStorageDomainId()));
getParameters().getDestDomainId()));
}
return true;
}
Expand All @@ -678,7 +678,7 @@ public List<QuotaConsumptionParameter> getQuotaStorageConsumptionParameters() {
list.add(new QuotaStorageConsumptionParameter(
getDestinationQuotaId(),
QuotaConsumptionParameter.QuotaAction.CONSUME,
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
(double)getImage().getSizeInGigabytes()));

if (isMoveOperation()) {
Expand Down Expand Up @@ -737,7 +737,7 @@ private boolean isCopyOperation() {
}

private boolean isManagedBlockCopy() {
return storageDomainDao.get(getParameters().getStorageDomainId()).getStorageType() == StorageType.MANAGED_BLOCK_STORAGE;
return storageDomainDao.get(getParameters().getDestDomainId()).getStorageType() == StorageType.MANAGED_BLOCK_STORAGE;
}

protected QuotaValidator createQuotaValidator(Guid quotaId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ protected void executeCommand() {
if (!shouldUpdateStorageDisk() && getParameters().getAddImageDomainMapping()) {
imageStorageDomainMapDao.save
(new ImageStorageDomainMap(getParameters().getImageId(),
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getQuotaId(),
getParameters().getDiskProfileId()));
}
Expand All @@ -158,7 +158,7 @@ private CopyImageGroupWithDataCommandParameters createCopyParams(Guid sourceDoma
CopyImageGroupWithDataCommandParameters p = new CopyImageGroupWithDataCommandParameters(
getStorageDomain().getStoragePoolId(),
sourceDomainId,
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getImageGroupID(),
getParameters().getImageId(),
destImageGroupId,
Expand All @@ -173,7 +173,7 @@ private CopyImageGroupWithDataCommandParameters createCopyParams(Guid sourceDoma
p.setEndProcedure(EndProcedure.COMMAND_MANAGED);
p.setDestImages(getParameters().getDestImages());
p.setJobWeight(getParameters().getJobWeight());
p.setDestDomain(getParameters().getStorageDomainId());
p.setDestDomain(getParameters().getDestDomainId());

return p;
}
Expand Down Expand Up @@ -213,7 +213,7 @@ private boolean performStorageOperation() {
getParameters().getDestImageGroupId(),
getParameters().getDestinationImageId(),
"",
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getCopyVolumeType(),
getVolumeFormatForDomain(),
getParameters().getVolumeType(),
Expand All @@ -230,7 +230,7 @@ private boolean performStorageOperation() {
sourceDomainId,
getDiskImage() != null ?
getDiskImage().getId() : getParameters().getImageGroupID(),
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getContainerId(),
ImageOperation.Copy,
isWipeAfterDelete(),
Expand All @@ -246,7 +246,7 @@ private boolean performStorageOperation() {
getParameters().getParentCommand(),
VdcObjectType.Storage,
sourceDomainId,
getParameters().getStorageDomainId()));
getParameters().getDestDomainId()));
}

return vdsReturnValue.getSucceeded();
Expand Down Expand Up @@ -278,7 +278,7 @@ private VolumeFormat getVolumeFormatForDomain() {
return VolumeFormat.COW;
}

StorageDomainStatic destDomain = storageDomainStaticDao.get(getParameters().getStorageDomainId());
StorageDomainStatic destDomain = storageDomainStaticDao.get(getParameters().getDestDomainId());
if (destDomain.getStorageType().isBlockDomain() && getParameters().getVolumeType() == VolumeType.Sparse) {
return VolumeFormat.COW;
} else {
Expand Down Expand Up @@ -321,7 +321,7 @@ protected void endSuccessfully() {
snapshot.getStorageIds().get(0)));
imageStorageDomainMapDao.save
(new ImageStorageDomainMap(snapshot.getImageId(),
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getQuotaId(),
getParameters().getDiskProfileId()));
setQcowCompatForSnapshot(snapshot, null);
Expand All @@ -336,15 +336,15 @@ protected void setQcowCompatForSnapshot(DiskImage snapshot, DiskImage volInfo) {
try {
if (info == null) {
info = getVolumeInfo(snapshot.getStoragePoolId(),
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
snapshot.getId(),
snapshot.getImageId());
}
if (info != null) {
setQcowCompatByQemuImageInfo(snapshot.getStoragePoolId(),
snapshot.getId(),
snapshot.getImageId(),
getParameters().getStorageDomainId(),
getParameters().getDestDomainId(),
snapshot);
}
imageDao.update(snapshot.getImage());
Expand All @@ -353,13 +353,13 @@ protected void setQcowCompatForSnapshot(DiskImage snapshot, DiskImage volInfo) {
log.error("Unable to update the image info for image '{}' (image group: '{}') on domain '{}'",
snapshot.getImageId(),
snapshot.getId(),
getParameters().getStorageDomainId());
getParameters().getDestDomainId());
}
}
}

private boolean shouldUpdateStorageDisk() {
if (storageDomainDao.get(getParameters().getStorageDomainId()).getStorageType() == StorageType.MANAGED_BLOCK_STORAGE) {
if (storageDomainDao.get(getParameters().getDestDomainId()).getStorageType() == StorageType.MANAGED_BLOCK_STORAGE) {
return false;
}

Expand All @@ -377,7 +377,7 @@ protected void endWithFailure() {
// remove image-storage mapping
imageStorageDomainMapDao.remove
(new ImageStorageDomainMapId(getParameters().getImageId(),
getParameters().getStorageDomainId()));
getParameters().getDestDomainId()));
}
revertTasks();
setSucceeded(true);
Expand All @@ -395,7 +395,7 @@ protected void revertTasks() {
} else {
removeImageParams.setParentParameters(removeImageParams);
removeImageParams.setParentCommand(ActionType.RemoveImage);
removeImageParams.setStorageDomainId(getParameters().getStorageDomainId());
removeImageParams.setStorageDomainId(getParameters().getDestDomainId());
removeImageParams.setDbOperationScope(getParameters().getRevertDbOperationScope());
removeImageParams.setShouldLockImage(getParameters().isShouldLockImageOnRevert());
}
Expand Down Expand Up @@ -435,6 +435,6 @@ protected void unLockImage() {

private boolean isManagedBlockCopy() {
return storageDomainDao.get(getParameters().getSourceDomainId()).getStorageType() == StorageType.MANAGED_BLOCK_STORAGE ||
storageDomainDao.get(getParameters().getStorageDomainId()).getStorageType() == StorageType.MANAGED_BLOCK_STORAGE;
storageDomainDao.get(getParameters().getDestDomainId()).getStorageType() == StorageType.MANAGED_BLOCK_STORAGE;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ protected void endSuccessfully() {

@Override
protected void endWithFailure() {
removeImage(getParameters().getStorageDomainId());
removeImage(getParameters().getDestDomainId());
unLockImage();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ private CloneImageGroupVolumesStructureCommandParameters buildCloneImageGroupVol
CloneImageGroupVolumesStructureCommandParameters p =
new CloneImageGroupVolumesStructureCommandParameters(getParameters().getStoragePoolId(),
getParameters().getSourceDomainId(),
getParameters().getTargetStorageDomainId(),
getParameters().getDestDomainId(),
getImageGroupId(),
getActionType(),
getParameters());
Expand Down Expand Up @@ -404,20 +404,20 @@ protected void endWithFailure() {
private boolean completeLiveMigration() {
// Update the DB before sending the command (perform rollback on failure)
moveDiskInDB(getParameters().getSourceDomainId(),
getParameters().getTargetStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getQuotaId(),
getParameters().getDiskProfileId());

try {
replicateDiskFinish(getParameters().getSourceDomainId(), getParameters().getTargetStorageDomainId());
replicateDiskFinish(getParameters().getSourceDomainId(), getParameters().getDestDomainId());
} catch (Exception e) {
if (e instanceof EngineException &&
EngineError.unavail.equals(((EngineException) e).getErrorCode())) {
log.warn("Replication not finished yet, will retry in next polling cycle");
return false;
}

moveDiskInDB(getParameters().getTargetStorageDomainId(),
moveDiskInDB(getParameters().getDestDomainId(),
getParameters().getSourceDomainId(),
sourceQuotaId,
sourceDiskProfileId);
Expand Down Expand Up @@ -490,7 +490,7 @@ private void updateImagesInfo() {
VDSReturnValue ret = runVdsCommand(
VDSCommandType.GetImageInfo,
new GetImageInfoVDSCommandParameters(getParameters().getStoragePoolId(),
getParameters().getTargetStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getImageGroupID(),
image.getImageId()));
DiskImage imageFromIRS = (DiskImage) ret.getReturnValue();
Expand Down Expand Up @@ -519,7 +519,7 @@ private void syncImageData() {
new CopyImageGroupVolumesDataCommandParameters(getParameters().getStoragePoolId(),
getParameters().getSourceDomainId(),
getParameters().getImageGroupID(),
getParameters().getTargetStorageDomainId(),
getParameters().getDestDomainId(),
getActionType(),
getParameters());

Expand Down Expand Up @@ -551,7 +551,7 @@ private void replicateDiskStart() {
getParameters().getVmId(),
getParameters().getStoragePoolId(),
getParameters().getSourceDomainId(),
getParameters().getTargetStorageDomainId(),
getParameters().getDestDomainId(),
getParameters().getImageGroupID(),
getParameters().getDestinationImageId(),
diskType.orElse(null));
Expand Down Expand Up @@ -650,7 +650,7 @@ protected boolean validateCreateSnapshotForVmCommand() {

private StorageDomain getDstStorageDomain() {
if (dstStorageDomain == null) {
dstStorageDomain = storageDomainDao.getForStoragePool(getParameters().getTargetStorageDomainId(),
dstStorageDomain = storageDomainDao.getForStoragePool(getParameters().getDestDomainId(),
getStoragePoolId());
}
return dstStorageDomain;
Expand Down Expand Up @@ -775,7 +775,7 @@ private void cleanupDestDiskAfterFailure() {

log.error("Attempting to delete the target of disk '{}' of vm '{}'",
getParameters().getImageGroupID(), getParameters().getVmId());
removeImage(getParameters().getTargetStorageDomainId(),
removeImage(getParameters().getDestDomainId(),
getParameters().getImageGroupID(),
getParameters().getDestinationImageId(),
AuditLogType.USER_MOVE_IMAGE_GROUP_FAILED_TO_DELETE_DST_IMAGE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@ public LiveMigrateDiskParameters(Guid imageId,
setDiskProfileId(diskProfileId);
}

public Guid getTargetStorageDomainId() {
return getStorageDomainId();
}

private Guid vmId;

public Guid getVmId() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,17 @@ public MoveOrCopyImageGroupParameters(Guid imageId,
ImageOperation operation) {
super(imageId);
setSourceDomainId(sourceDomainId);
setStorageDomainId(destDomainId);
setDestDomainId(destDomainId);
setOperation(operation);
volumeFormat = VolumeFormat.UNUSED0;
volumeType = VolumeType.Unassigned;
copyVolumeType = CopyVolumeType.SharedVol;
}

public MoveOrCopyImageGroupParameters(Guid containerId, Guid imageGroupId, Guid leafSnapshotID,
Guid storageDomainId, ImageOperation operation) {
Guid destDomainId, ImageOperation operation) {
super(leafSnapshotID, containerId);
setStorageDomainId(storageDomainId);
setDestDomainId(destDomainId);
setImageGroupID(imageGroupId);
setOperation(operation);
setUseCopyCollapse(false);
Expand Down Expand Up @@ -163,6 +163,14 @@ public void setSourceDomainId(Guid sourceDomainId) {
this.sourceDomainId = sourceDomainId;
}

public Guid getDestDomainId() {
return getStorageDomainId();
}

public void setDestDomainId(Guid destDomainId) {
setStorageDomainId(destDomainId);
}

public ImageDbOperationScope getRevertDbOperationScope() {
return revertDbOperationScope;
}
Expand Down

0 comments on commit 63ca05c

Please sign in to comment.