Liron Ar has posted comments on this change. Change subject: core: handle 'source' domains when adding a vm from template ......................................................................
Patch Set 5: (7 comments) 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 if verifySourceDomains() fails, false will be returned (line 423) and you won't get to chooseDisksSourceDomains() 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 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 i'll add it. 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. this is not wrong, the disks in diskInfoDestinationMap contains on the Storage Ids list only the destination domain. this is not code that i introduced, it was just exported to this method - see the original file line 104. I agree that this could be changed, but it's not related to this change. 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/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 Done Line 193: Map<Guid, StorageDomain> storageDomains, Line 194: VdcBllMessages message, Line 195: Set<StorageDomainStatus> applicableStatuses) { Line 196: 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 Done 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 th Done 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 Done 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: Liron Ar <lara...@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
