Liron Ar has posted comments on this change. Change subject: core: Intrdoucing UploadStream capabillity ......................................................................
Patch Set 6: (19 comments) http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UploadStreamCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UploadStreamCommand.java: Line 42: Line 43: @Override Line 44: protected boolean canDoAction() { Line 45: if (getPoolSpmId() == null) { Line 46: // add message that there's no spm for the pool > please do. Done Line 47: return false; Line 48: } Line 49: Line 50: setStoragePool(null); Line 46: // add message that there's no spm for the pool Line 47: return false; Line 48: } Line 49: Line 50: setStoragePool(null); > why is this required? to reload the pool, actually i have debates on wether we better reduce the checks here or not because of socket possibly related bug. in the meanwhile i added the needed messages. Line 51: if (getStoragePool() == null) { Line 52: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST); Line 53: } Line 54: Line 56: return false; Line 57: } Line 58: Line 59: if (!getPoolSpmId().equals(getStoragePool().getspm_vds_id())) { Line 60: // add message that the host isn't the spm > please do. Done Line 61: return false; Line 62: } Line 63: Line 64: VdsDynamic vdsDynamic = getVdsDynamicDao().get(getPoolSpmId()); Line 67: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL); Line 68: } Line 69: Line 70: DiskImage targetDisk = getDiskImage(); Line 71: //Currently we'd like to support only preallocated disks to avoid possible extend on vdsm side. > s/extend/extend requests/ extend requests isn't correct for NFS. Line 72: if (targetDisk.getVolumeType() != VolumeType.Preallocated) { Line 73: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_VOLUME_TYPE_UNSUPPORTED, Line 74: String.format("$volumeType %2$s", targetDisk.getVolumeFormat()), Line 75: String.format("$supportedVolumeTypes %1$s", VolumeType.Preallocated)); Line 72: if (targetDisk.getVolumeType() != VolumeType.Preallocated) { Line 73: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_VOLUME_TYPE_UNSUPPORTED, Line 74: String.format("$volumeType %2$s", targetDisk.getVolumeFormat()), Line 75: String.format("$supportedVolumeTypes %1$s", VolumeType.Preallocated)); Line 76: } > I'd also check that it's RAW not sure about that, the upload is supported for any volumes that you'd specify. this check is done here because we want to avoid upload extensions at the moment. but perhaps the consequences of upload to something other than raw is the responsibility of the caller. Line 77: Line 78: setVdsId(vdsDynamic.getId()); Line 79: Line 80: return true; Line 74: String.format("$volumeType %2$s", targetDisk.getVolumeFormat()), Line 75: String.format("$supportedVolumeTypes %1$s", VolumeType.Preallocated)); Line 76: } Line 77: Line 78: setVdsId(vdsDynamic.getId()); > This should be above, coupled to the VdsDynamic check Done Line 79: Line 80: return true; Line 81: } Line 82: Line 80: return true; Line 81: } Line 82: Line 83: @Override Line 84: protected void executeCommand() { > should we start by creating a task placeholder, as per the new API? being done @ line 32. Line 85: UploadStreamVDSCommandParameters vdsCommandParameters = Line 86: new UploadStreamVDSCommandParameters( Line 87: getVdsId(), Line 88: getParameters().getStoragePoolId(), Line 129: new Pair<>(LockingGroup.VDS_EXECUTION.toString(), Line 130: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED.toString())); Line 131: } Line 132: return null; Line 133: } > I'd also add an exclusive lock on the disk. currently the lock is taken by the initiating command that does further operations before and after the upload (the upload command is used only for "uploading", therefore it should be called when the resources are already lock). see here @ line 115: http://gerrit.ovirt.org/#/c/23529/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProccessOvfUpdateForStorageDomainCommand.java http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/bll/UploadStreamParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/bll/UploadStreamParameters.java: Line 7: import org.ovirt.engine.core.compat.Guid; Line 8: Line 9: public class UploadStreamParameters extends ImagesContainterParametersBase { Line 10: @JsonIgnore Line 11: ByteArrayInputStream inputStream; > If you have the length, you aren't really treating this as a stream. Why no it's a leftover, this is an InputStream, the length is used to tell us how much to read from it. Line 12: Line 13: @JsonIgnore Line 14: Long streamLength; Line 15: http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java: Line 303: AddInternalJob(1850, false, QuotaDependency.NONE), Line 304: AddInternalStep(1851, false, QuotaDependency.NONE), Line 305: Line 306: UpdateMomPolicy(1900, ActionGroup.MANIPUTLATE_HOST, false, QuotaDependency.NONE), Line 307: UploadStream(1901, QuotaDependency.NONE); > shouldn't this depend on DISK quota? we have a quota dependency of STORAGE which is automatically used when the ovf disk is created by the AddDiskCommand. Line 308: Line 309: private int intValue; Line 310: private ActionGroup actionGroup; Line 311: private boolean isActionMonitored; http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/asynctasks/AsyncTaskType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/asynctasks/AsyncTaskType.java: Line 10: mergeSnapshots, Line 11: cloneImageStructure, Line 12: syncImageData, Line 13: extendImageSize, Line 14: downloadImageFromStream; > download? not upload? the term on vdsm side is "downloadImageFromStream". Line 15: Line 16: public int getValue() { Line 17: return this.ordinal(); Line 18: } http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/constants/StorageConstants.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/constants/StorageConstants.java: Line 2: Line 3: public class StorageConstants { Line 4: public static final int SIZE_IS_NOT_AVAILABLE = -1; Line 5: public static final int OVF_MAX_ITEMS_PER_SQL_STATEMENT = 100; Line 6: public static final int UPLOAD_SOCKET_TIMEOUT = 60; > both should be conf values as of today, OVF_MAX_ITEMS_PER_SQL_STATEMENT isn't a config value but a constant in OvfDataUpdater. if you don't mind - let's change it to a config value in a separate change. UPLOAD_SOCKET_TIMEOUT isn't relevant anymore as it was removed, i'll apply the removal to this patch so it won't be seen here. http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/locks/LockingGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/locks/LockingGroup.java: Line 21: /** This group is used to lock template which is in export domain */ Line 22: REMOTE_TEMPLATE, Line 23: /** This group is used to lock VM which is in export domain */ Line 24: REMOTE_VM, Line 25: VDS_EXECUTION; > perhaps add a small javadoc line? Done Line 26: http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/UploadStreamVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/UploadStreamVDSCommandParameters.java: Line 6: import org.ovirt.engine.core.compat.Guid; Line 7: Line 8: public class UploadStreamVDSCommandParameters extends VdsIdVDSCommandParametersBase { Line 9: InputStream inputStream; Line 10: Long streamLength; > same comment - if you have a stream+length it can simply be a byte[] the stream is an inputstream, if we want to read and upload dynamically from the stream for some length, it can't be byte[] (all is in memory)..for example, from FileInputStream. Line 11: Guid storagePoolId; Line 12: Guid storageDomainId; Line 13: Guid imageGroupId; Line 14: Guid imageId; Line 10: Long streamLength; Line 11: Guid storagePoolId; Line 12: Guid storageDomainId; Line 13: Guid imageGroupId; Line 14: Guid imageId; > These should all be private Done Line 15: Line 16: public UploadStreamVDSCommandParameters(Guid vdsId, Guid storagePoolId, Guid storageDomainId, Guid imageGroupId, Guid imageId, ByteArrayInputStream inputStream, Long streamLength) { Line 17: super(vdsId); Line 18: this.inputStream = inputStream; http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/UploadStreamVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/UploadStreamVDSCommand.java: Line 39: import org.ovirt.engine.core.vdsbroker.xmlrpc.XmlRpcUtils; Line 40: Line 41: public class UploadStreamVDSCommand<P extends UploadStreamVDSCommandParameters> extends VdsBrokerCommand<P> { Line 42: Line 43: private static Log log = LogFactory.getLog(UploadStreamVDSCommand.class); > should be static it is static. Line 44: private EngineLock lock; Line 45: Line 46: public UploadStreamVDSCommand(P parameters) { Line 47: super(parameters); Line 40: Line 41: public class UploadStreamVDSCommand<P extends UploadStreamVDSCommandParameters> extends VdsBrokerCommand<P> { Line 42: Line 43: private static Log log = LogFactory.getLog(UploadStreamVDSCommand.class); Line 44: private EngineLock lock; > lock in a VDSCommand? can't be take it higher up? it's a leftover from a test version, it's never used..removed. Line 45: Line 46: public UploadStreamVDSCommand(P parameters) { Line 47: super(parameters); Line 48: } Line 72: final PutMethod putMethod = Line 73: new PutMethod(urlInfo.getFirst()); Line 74: try { Line 75: InputStreamRequestEntity inputStreamRequestEntity = null; Line 76: if (getParameters().getStreamLength() != null) { > when will you ever want this to be null? it can be null - when we don't know it (read til the end of the stream) Line 77: inputStreamRequestEntity = Line 78: new InputStreamRequestEntity(getParameters().getInputStream(), Line 79: getParameters().getStreamLength()); Line 80: } else { http://gerrit.ovirt.org/#/c/23460/6/packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql File packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql: Line 229: select fn_db_add_config_value('HotPlugDiskSnapshotSupported','false','3.2'); Line 230: select fn_db_add_config_value('OvfOnAnyDomain','false','3.0'); Line 231: select fn_db_add_config_value('OvfOnAnyDomain','false','3.1'); Line 232: select fn_db_add_config_value('OvfOnAnyDomain','false','3.2'); Line 233: select fn_db_add_config_value('OvfOnAnyDomain','false','3.3'); > Let's find a better name for this - the point isn't having an OVF on any do yep, it should be on this patch..it's used only on the patch that actually adds this feature. removed from here and as you asked - change to "OvfStoreOnAnyDomain" Line 234: Line 235: -- by default use no proxy Line 236: select fn_db_add_config_value('SpiceProxyDefault','','general'); Line 237: -- To view, visit http://gerrit.ovirt.org/23460 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7af6e7b580e8844c1f0cd661d7f62ece688e31d3 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[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
