Maor Lipchuk has posted comments on this change. Change subject: core: Improve attach process when register disks ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/33154/1//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-09-22 00:59:29 +0300 Line 6: Line 7: core: Improve attach process when register disks Line 8: Line 9: Remove redundant failure log when disk is failed to be registered > s/disk is failed to be registered/failed to register disk/ done Line 10: as part of the attach process. Line 11: Disks will not be registered if they have snapshots, for example, Line 12: although the attach process will still succeed and should not be related Line 13: to this. Line 12: although the attach process will still succeed and should not be related Line 13: to this. Line 14: Line 15: As part of the fix, there is also an improvement of the images registration Line 16: flow, this are the improvements: > As part of the fix, there are also several improvement to the images regist done Line 17: 1. Avoid redundant validation for Storage Domain availability, since it is Line 18: already being validated in the CDA Line 19: 2. Avoid feching for existing images in the Storage Domain, since the Storage Domain was only Line 20: attached and does not contain any images in the setup yet. Line 16: flow, this are the improvements: Line 17: 1. Avoid redundant validation for Storage Domain availability, since it is Line 18: already being validated in the CDA Line 19: 2. Avoid feching for existing images in the Storage Domain, since the Storage Domain was only Line 20: attached and does not contain any images in the setup yet. > But what if it's a template image with several copies? if we had a Template with several copies we will still won't have any indication in the setup that there is a copy of that disk in the Storage Domain. Currently all we do in this specific process is to look for OVF_disks for now. also, the OVF does not support Template with several copies for now Line 21: 3. Avoid calling two internal query command. Line 22: Line 23: Change-Id: If302d623ab13217776e0cb6ec49dda9cc59b7b39 Line 24: Bug-Url: https://bugzilla.redhat.com/1136808 Line 17: 1. Avoid redundant validation for Storage Domain availability, since it is Line 18: already being validated in the CDA Line 19: 2. Avoid feching for existing images in the Storage Domain, since the Storage Domain was only Line 20: attached and does not contain any images in the setup yet. Line 21: 3. Avoid calling two internal query command. > s/command/commands/ done Line 22: Line 23: Change-Id: If302d623ab13217776e0cb6ec49dda9cc59b7b39 Line 24: Bug-Url: https://bugzilla.redhat.com/1136808 http://gerrit.ovirt.org/#/c/33154/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AttachStorageDomainToPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AttachStorageDomainToPoolCommand.java: Line 290: return unregisteredDisks; Line 291: } Line 292: imagesFromVds = (List<Guid>) vdsReturnValue.getReturnValue(); Line 293: } catch (VdcBLLException e) { Line 294: log.warnFormat("Failed to get a list of images for Storage Domain to register OVF disks. Error: {0}", e); > shouldn't we just return here? unregisteredDisks will be empty, and the for yes, done Line 295: } Line 296: for (Guid unregisteredDiskId : imagesFromVds) { Line 297: populateUnregisteredDisksListWithUnregisteredDisk(storagePoolId, Line 298: storageDomainId, Line 324: Line 325: // We can't deal with snapshots, so there should only be a single volume associated with the Line 326: // image. If there are multiple volumes, skip the image and move on to the next. Line 327: if (volumesList.size() != 1) { Line 328: log.info("Could not get populated disk since the disk contains snapshots."); > add the disk's guid. We can know the guid from the log of the previous VDscommand GetVolumesList, but I guess it can only help if I will add it Line 329: return; Line 330: } Line 331: Line 332: // Get the information about the volume from VDSM. -- To view, visit http://gerrit.ovirt.org/33154 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If302d623ab13217776e0cb6ec49dda9cc59b7b39 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
