Maor Lipchuk has posted comments on this change. Change subject: core: Move storage type checks from storage pool level to domain level ......................................................................
Patch Set 4: Code-Review-1 (13 comments) please seperate GUI changes with engine changes http://gerrit.ovirt.org/#/c/23072/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDiskAlignmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDiskAlignmentCommand.java: Line 115: Line 116: StorageDomainStatic sds = getStorageDomainStaticDAO().get(((DiskImage)getDisk()).getStorageIds().get(0)); Line 117: if (!sds.getStorageType().isBlockDomain()) { Line 118: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_ALIGNMENT_SCAN_STORAGE_TYPE); Line 119: } This is only relevant for images it should be in the DiskImagesValidator Line 120: } Line 121: Line 122: if (isImageExclusiveLockNeeded() && getVm().isRunningOrPaused()) { Line 123: return failCanDoAction(VdcBllMessages.ERROR_CANNOT_RUN_ALIGNMENT_SCAN_VM_IS_RUNNING); http://gerrit.ovirt.org/#/c/23072/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HibernateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HibernateVmCommand.java: Line 380: * efficient to use Sparse allocation. Otherwise for block devices we should use Preallocated for faster allocation. Line 381: * Line 382: * @return - VolumeType of allocation type to use. Line 383: */ Line 384: public static VolumeType getMemoryVolumeTypeForPool(StorageType storageType) { The name of the method is wrong, you getMemoryVolumeFor StorageDomain not for pool anymore Line 385: return storageType.isFileDomain() ? VolumeType.Sparse : VolumeType.Preallocated; Line 386: } Line 387: Line 388: private static Log log = LogFactory.getLog(HibernateVmCommand.class); http://gerrit.ovirt.org/#/c/23072/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java: Line 714: } Line 715: Line 716: private MoveOrCopyImageGroupParameters buildMoveOrCopyImageGroupParametersForMemoryDumpImage(Guid containerID, Line 717: Guid storageId, Guid imageId, Guid volumeId) { Line 718: empty line is redundant for this patch Line 719: MoveOrCopyImageGroupParameters params = new MoveOrCopyImageGroupParameters(containerID, Line 720: imageId, volumeId, imageId, volumeId, storageId, getMoveOrCopyImageOperation()); Line 721: params.setParentCommand(getActionType()); Line 722: params.setCopyVolumeType(CopyVolumeType.LeafVol); Line 727: params.setEntityInfo(new EntityInfo(VdcObjectType.VM, getVm().getId())); Line 728: params.setParentParameters(getParameters()); Line 729: Line 730: StorageDomainStatic storageDomain = getStorageDomainStaticDAO().get(storageId); Line 731: if (storageDomain != null && storageDomain.getStorageType().isBlockDomain()) { If storage domain is null we have a bug and we should add an error log about it Line 732: params.setUseCopyCollapse(true); Line 733: params.setVolumeType(VolumeType.Preallocated); Line 734: params.setVolumeFormat(VolumeFormat.RAW); Line 735: } http://gerrit.ovirt.org/#/c/23072/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/LiveSnapshotMemoryImageBuilder.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/LiveSnapshotMemoryImageBuilder.java: Line 90: storagePool.getId(), Line 91: storageDomain.getId(), Line 92: memoryDumpImageGroupId, Line 93: vm.getTotalMemorySizeInBytes(), Line 94: HibernateVmCommand.getMemoryVolumeTypeForPool(storageDomain.getStorageType()), The method name getMemoryVolumeTypeForPool is wrong now, you should call it getMemoryVolumeTypeForDomain Line 95: VolumeFormat.RAW, Line 96: memoryDumpVolumeId, Line 97: "")); Line 98: http://gerrit.ovirt.org/#/c/23072/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectHostToStoragePooServerCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectHostToStoragePooServerCommandBase.java: Line 41 Line 42 Line 43 Line 44 Line 45 Why was this have been removed? Line 12: import org.ovirt.engine.core.common.businessentities.StorageType; Line 13: import org.ovirt.engine.core.dal.dbbroker.DbFacade; Line 14: Line 15: @InternalCommandAttribute Line 16: public abstract class ConnectHostToStoragePooServerCommandBase<T extends StoragePoolParametersBase> extends lol, nice... StoragePoo Would be nice if it can be fixed while you at it Line 17: StorageHandlingCommandBase<T> { Line 18: private List<StorageServerConnections> _connections; Line 19: private List<StorageServerConnections> _isoConnections; Line 20: private List<StorageServerConnections> _exportConnections; http://gerrit.ovirt.org/#/c/23072/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectHostToStoragePoolServersCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectHostToStoragePoolServersCommand.java: Line 46: boolean connectSucceeded = true; Line 47: if (connections != null && connections.size() > 0) { Line 48: Map<StorageType, List<StorageServerConnections>> connectionsToTypes = new HashMap<>(); Line 49: for (StorageServerConnections conn : connections) { Line 50: if (!connectionsToTypes.containsKey(conn.getstorage_type())) { please use MultiValueMapUtils.addToMap Line 51: connectionsToTypes.put(conn.getstorage_type(), new ArrayList<StorageServerConnections>()); Line 52: } Line 53: connectionsToTypes.get(conn.getstorage_type()).add(conn); Line 54: } Line 50: if (!connectionsToTypes.containsKey(conn.getstorage_type())) { Line 51: connectionsToTypes.put(conn.getstorage_type(), new ArrayList<StorageServerConnections>()); Line 52: } Line 53: connectionsToTypes.get(conn.getstorage_type()).add(conn); Line 54: } Please extend this to another method, this should be similar to StorageHelperBase.filterConnectionsByStorageType perhaps it is better to see if we can change filterConnectionsByStorageType for this case also. Line 55: Line 56: for (Map.Entry<StorageType, List<StorageServerConnections>> connectionToType : connectionsToTypes.entrySet()) { Line 57: connectSucceeded = connectSucceeded && connectStorageServersByType(connectionToType.getKey(), connectionToType.getValue()); Line 58: } Line 53: connectionsToTypes.get(conn.getstorage_type()).add(conn); Line 54: } Line 55: Line 56: for (Map.Entry<StorageType, List<StorageServerConnections>> connectionToType : connectionsToTypes.entrySet()) { Line 57: connectSucceeded = connectSucceeded && connectStorageServersByType(connectionToType.getKey(), connectionToType.getValue()); no point to stay in the for loop when the connect failed. You should simply print an error log and return false Line 58: } Line 59: Line 60: log.infoFormat("Host {0} storage connection was {1} ", getVds().getName(), Line 61: connectSucceeded ? "succeeded" : "failed"); Line 56: for (Map.Entry<StorageType, List<StorageServerConnections>> connectionToType : connectionsToTypes.entrySet()) { Line 57: connectSucceeded = connectSucceeded && connectStorageServersByType(connectionToType.getKey(), connectionToType.getValue()); Line 58: } Line 59: Line 60: log.infoFormat("Host {0} storage connection was {1} ", getVds().getName(), You should indicate which storage type did not succeed. Line 61: connectSucceeded ? "succeeded" : "failed"); Line 62: } Line 63: return connectSucceeded; Line 64: } Line 66: private boolean connectStorageServersByType(StorageType storageType, List<StorageServerConnections> connections) { Line 67: Map<String, String> retValues = (HashMap<String, String>) Backend Line 68: .getInstance() Line 69: .getResourceManager() Line 70: .RunVdsCommand( Use runVdsCommand instead Line 71: VDSCommandType.ConnectStorageServer, Line 72: new StorageServerConnectionManagementVDSParameters(getVds().getId(), Line 73: getStoragePool().getId(), storageType, connections)).getReturnValue(); Line 74: return StorageHelperDirector.getInstance().getItem(storageType).isConnectSucceeded(retValues, connections); http://gerrit.ovirt.org/#/c/23072/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetDiskAlignmentCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetDiskAlignmentCommandTest.java: Line 185: VdcBllMessages.ACTION_TYPE_FAILED_IMAGE_REPOSITORY_NOT_FOUND); Line 186: } Line 187: Line 188: @Test Line 189: public void testCanDoActionStoragePoolFile() { The name of the method is not relevant to the test, you are now testing it on storageDomain Line 190: storageDomain.setStorageType(StorageType.NFS); Line 191: CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd, Line 192: VdcBllMessages.ACTION_TYPE_FAILED_ALIGNMENT_SCAN_STORAGE_TYPE); Line 193: } -- To view, visit http://gerrit.ovirt.org/23072 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f1067fd1d299a93b9555c4714b4e85ff980a830 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tal Nisan <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Vered Volansky <[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
