Maor Lipchuk has posted comments on this change. Change subject: core: handle 'source' domains when adding a vm from template ......................................................................
Patch Set 5: Code-Review-1 (11 comments) http://gerrit.ovirt.org/#/c/26262/5//COMMIT_MSG Commit Message: Line 6: Line 7: core: handle 'source' domains when adding a vm from template Line 8: Line 9: When adding a vm from template we need to make sure to choose an Line 10: Active domain on which the template disks stored on for the operation- /s/operation-/operation Line 11: Line 12: 1. If there's no such domain, a CDA message should be displayed. Line 13: 2. If the source disk is stored on few domains, the engine will attempt to Line 14: select the same domain as the target domain for possibly faster copy. http://gerrit.ovirt.org/#/c/26262/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 422: if (!verifySourceDomains() || !buildAndCheckDestStorageDomains()) { Line 423: return false; Line 424: } Line 425: Line 426: chooseDisksSourceDomains(); I would prefer to call chooseDisksSourceDomains() only at the else scope of the if condition at line 422. The reason for that is that chooseDisksSourceDomains() is actually count on the succeed of verifySourceDomains() validation, and that might be confusing to maintain if it will not indicate it is related to it Line 427: Line 428: // otherwise.. Line 429: storageToDisksMap = Line 430: ImagesHandler.buildStorageToDiskMap(getImagesToCheckDestinationStorageDomains(), http://gerrit.ovirt.org/#/c/26262/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java: Line 21: import org.ovirt.engine.core.common.businessentities.VMStatus; Line 22: import org.ovirt.engine.core.common.errors.VdcBLLException; Line 23: import org.ovirt.engine.core.common.errors.VdcBllErrors; Line 24: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 25: import org.ovirt.engine.core.common.utils.Pair; Should be removed (Checkstyle violation) Line 26: import org.ovirt.engine.core.compat.Guid; Line 27: import org.ovirt.engine.core.dal.dbbroker.DbFacade; Line 28: import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector; Line 29: import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogableBase; Line 174: diskInfoSourceMap = new HashMap<>(); Line 175: for (DiskImage disk : getImagesToCheckDestinationStorageDomains()) { Line 176: Guid diskId = disk.getId(); Line 177: Set<Guid> validDomainsForDisk = validDisksDomains.get(diskId); Line 178: Guid destinationDomain = retrieveDestinationDomainForDisk(diskId); Please consider to add an empty line here, to make the code more readable Line 179: // if the destination domain is one of the valid source domains, we can Line 180: // choose the same domain as the source domain for Line 181: // possibly faster operation, otherwise we'll choose random valid domain as the source. Line 182: if (validDomainsForDisk.contains(destinationDomain)) { Line 187: } Line 188: } Line 189: Line 190: private Guid retrieveDestinationDomainForDisk(Guid id) { Line 191: return diskInfoDestinationMap.get(id).getStorageIds().get(0); The heuristic here is wrong. You explained in the commit message that the source disk is stored on few domains, and the engine will attempt to select the same domain as the target domain for possibly faster copy. but here it is not the case, you always choose the first storage domain for the first disk, who says it is the most common one for all the disks? How can u be sure that the first storage domain is also the same one for all the other disks. What you should do is to choose the storage domain which appears most in all the disks, or keep the code as it is but remove the comment from the commit message and line 179-181. Line 192: } Line 193: Line 194: @Override Line 195: protected boolean isVirtioScsiEnabled() { http://gerrit.ovirt.org/#/c/26262/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java: Line 468: Set<StorageDomainStatus> applicableStatuses) { Line 469: Map<Guid, Set<Guid>> disksApplicableDomainsMap = new HashMap<>(); Line 470: for (DiskImage diskImage : diskImages) { Line 471: Set<Guid> diskApplicableDomain = new HashSet<>(); Line 472: disksApplicableDomainsMap.put(diskImage.getId(), diskApplicableDomain); suggestion: I would prefer this line will be at the end of the for loop (after line 478). It is much more readable IMHO Line 473: for (Guid storageDomainID : diskImage.getStorageIds()) { Line 474: StorageDomain domain = storageDomains.get(storageDomainID); Line 475: if (applicableStatuses.contains(domain.getStatus())) { Line 476: diskApplicableDomain.add(domain.getId()); http://gerrit.ovirt.org/#/c/26262/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskImagesValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskImagesValidator.java: Line 188: /** Line 189: * Checks that the given disks has no derived disks on the given storage domain. if the provided storage domain id Line 190: * is null, it will be checked that there are no derived disks on any storage domain. Line 191: */ Line 192: public ValidationResult diskImagesOnAnyApplicableDomains(Map<Guid, Set<Guid>> validDomainsForDisk, Please add a test to DiskImagesValidatorTest Line 193: Map<Guid, StorageDomain> storageDomains, Line 194: VdcBllMessages message, Line 195: Set<StorageDomainStatus> applicableStatuses) { Line 196: Line 216: ValidationResult result = ValidationResult.VALID; Line 217: Line 218: if (!disksInfo.isEmpty()) { Line 219: result = Line 220: new ValidationResult(message, Please consider to do this at the same line as so: result = new ValidationResult(message,.... Line 221: String.format("$disksInfo %s", Line 222: String.format(StringUtils.join(disksInfo, "%n"))), Line 223: String.format("$applicableStatus %s", StringUtils.join(applicableStatuses, ","))); Line 224: } Line 218: if (!disksInfo.isEmpty()) { Line 219: result = Line 220: new ValidationResult(message, Line 221: String.format("$disksInfo %s", Line 222: String.format(StringUtils.join(disksInfo, "%n"))), please remove the %n, you are already added it at line 211 Line 223: String.format("$applicableStatus %s", StringUtils.join(applicableStatuses, ","))); Line 224: } Line 225: Line 226: return result; http://gerrit.ovirt.org/#/c/26262/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 641: ACTION_TYPE_FAILED_DOMAIN_OVF_ON_UPDATE(ErrorType.CONFLICT), Line 642: ACTION_TYPE_FAILED_DISK_IS_BEING_MIGRATED(ErrorType.CONFLICT), Line 643: ACTION_TYPE_FAILED_DESTINATION_AND_SOURCE_STORAGE_SUB_TYPES_DIFFERENT(ErrorType.CONFLICT), Line 644: ACTION_TYPE_FAILED_TEMPLATE_IS_BEING_EXPORTED(ErrorType.CONFLICT), Line 645: ACTION_TYPE_FAILED_TEMPLATE_DISKS_NO_FIT_DOMAIN(ErrorType.CONFLICT), TEMPLATE_DISKS_NO_FIT_DOMAIN might be misleading perhaps worth to change this to ACTION_TYPE_FAILED_NO_VALID_DOMAINS_STATUS_FOR_TEMPLATE_DISKS Line 646: ACTION_TYPE_FAILED_VM_IS_BEING_MIGRATED(ErrorType.CONFLICT), Line 647: ACTION_TYPE_FAILED_VM_IS_BEING_IMPORTED(ErrorType.CONFLICT), Line 648: ACTION_TYPE_FAILED_TEMPLATE_IS_BEING_REMOVED(ErrorType.CONFLICT), Line 649: ACTION_TYPE_FAILED_VM_IS_BEING_REMOVED_FROM_EXPORT_DOMAIN(ErrorType.CONFLICT), http://gerrit.ovirt.org/#/c/26262/5/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 651: ACTION_TYPE_FAILED_DOMAIN_OVF_ON_UPDATE=Cannot ${action} ${type}. Storage Domain OVF data is currently being updated. Line 652: ACTION_TYPE_FAILED_DISK_IS_BEING_MIGRATED=Cannot ${action} ${type}. Disk ${DiskName} is being moved or copied. Line 653: ACTION_TYPE_FAILED_DESTINATION_AND_SOURCE_STORAGE_SUB_TYPES_DIFFERENT=Cannot ${action} ${type}. Source and target domains must both be either file domains or block domains. Line 654: ACTION_TYPE_FAILED_TEMPLATE_IS_BEING_EXPORTED=Cannot ${action} ${type}. Template ${TemplateName} is being exported. Line 655: ACTION_TYPE_FAILED_TEMPLATE_DISKS_NO_FIT_DOMAIN=Cannot ${action} ${type}. Can't find Domain(s) in ${applicableStatus} status for all of the Template disks.\n\ /s/for all/for some Line 656: Please make sure that there is at least one Storage Domain in applicable status for each one of the disks :\n\ Line 657: ${disksInfo}. Line 658: ACTION_TYPE_FAILED_VM_IS_BEING_IMPORTED=Cannot ${action} ${type}. VM ${VmName} is being imported. Line 659: ACTION_TYPE_FAILED_VM_IS_BEING_MIGRATED=Cannot ${action} ${type}. VM ${VmName} is being migrated. -- To view, visit http://gerrit.ovirt.org/26262 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8d02cfded41a3588dc944d9dfcd5a3eec88c45ab Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
