Liron Ar has posted comments on this change.
Change subject: [wip] backend: add the import glance image support
......................................................................
Patch Set 2: (14 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportRepoImageCommand.java
Line 23: import java.util.Arrays;
Line 24: import java.util.List;
Line 25:
Line 26: @SuppressWarnings("unused")
Line 27: public class ImportRepoImageCommand<T extends
ImportRepoImageParameters> extends CommandBase<T>
this command should be marked as non transactive
Line 28: implements TaskHandlerCommand<ImportRepoImageParameters>,
QuotaStorageDependent {
Line 29:
Line 30: private OpenstackImageProviderProxy providerProxy;
Line 31:
Line 99: }
Line 100:
Line 101: @Override
Line 102: protected void setActionMessageParameters() {
Line 103: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__MOVE);
perhaps change to import instead of move
Line 104: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__VM_DISK);
Line 105: }
Line 106:
Line 107: @Override
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportRepoImageCopyTaskHandler.java
Line 25: public ImportRepoImageCopyTaskHandler(TaskHandlerCommand<? extends
ImportRepoImageParameters> cmd) {
Line 26: super(cmd);
Line 27: }
Line 28:
Line 29: private OpenstackImageProviderProxy providerProxy;
please move the deceleration to the top
Line 30:
Line 31: @Override
Line 32: protected void beforeTask() {
Line 33: }
Line 41: public AsyncTaskType getTaskType() {
Line 42: return AsyncTaskType.copyImage;
Line 43: }
Line 44:
Line 45: protected OpenstackImageProviderProxy getProviderProxy() {
1. consider have exported to a helper class, as this code seems to appear in
few places such as ImportRepoImageCommand
2. you can get just the StorageDomainStatic, it's much lighter query that
doesn't involves joins - unlink getStorageDomain
3. consider pass the provider in the parameters.
Line 46: if (providerProxy == null) {
Line 47: StorageDomain storageDomain =
DbFacade.getInstance().getStorageDomainDao().get(
Line 48:
getEnclosingCommand().getParameters().getSourceStorageDomainId());
Line 49: Provider provider =
DbFacade.getInstance().getProviderDao().get(new
Guid(storageDomain.getStorage()));
Line 57: HashMap<String,String> httpHeaders = new HashMap<>();
Line 58: String XAuthToken = getProviderProxy().getXAuthToken();
Line 59:
Line 60: if (XAuthToken != null) {
Line 61: httpHeaders.put("X-Auth-Token", XAuthToken);
consider have helper class/method to handle it
Line 62: }
Line 63:
Line 64: String imageUrl =
getProviderProxy().getImageUrl(getEnclosingCommand().getParameters().getSourceRepoImageId());
Line 65:
Line 85: @Override
Line 86: protected void revertTask() {
Line 87:
getEnclosingCommand().getParameters().getDiskImage().setImageStatus(ImageStatus.ILLEGAL);
Line 88: ImagesHandler.updateImageStatus(
Line 89:
getEnclosingCommand().getParameters().getDestinationImageId(),
ImageStatus.ILLEGAL);
please perform the update explicitly on the image that you modified in the line
before (add .getId)
Line 90: }
Line 91:
Line 92: @Override
Line 93: public void endSuccessfully() {
Line 93: public void endSuccessfully() {
Line 94:
getEnclosingCommand().getParameters().getDiskImage().setImageStatus(ImageStatus.OK);
Line 95: ImagesHandler.updateImageStatus(
Line 96:
getEnclosingCommand().getParameters().getDestinationImageId(), ImageStatus.OK);
Line 97: getReturnValue().setSucceeded(true);
same
Line 98: }
Line 99:
Line 100: @Override
Line 101: public void endWithFailure() {
Line 113: return AsyncTaskType.deleteImage;
Line 114: }
Line 115:
Line 116: @Override
Line 117: protected VDSParametersBase getRevertVDSParameters() {
perhaps perform the revert through RemoveImageCommand in revertTask() - that
way the image would be removed completely in that case (also from the DB) i and
we will benefit from the additional logic in RemoveImage.
Line 118: DeleteImageGroupVDSCommandParameters parameters = new
DeleteImageGroupVDSCommandParameters();
Line 119:
parameters.setStoragePoolId(getEnclosingCommand().getParameters().getStoragePoolId());
Line 120:
parameters.setStorageDomainId(getEnclosingCommand().getParameters().getStorageDomainId());
Line 121:
parameters.setImageGroupId(getEnclosingCommand().getParameters().getImageGroupID());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportRepoImageCreateTaskHandler.java
Line 34: }
Line 35:
Line 36: @Override
Line 37: public void execute() {
Line 38: if (enclosingCommand.getParameters().getTaskGroupSuccess()) {
can you remind me why we can't call here AddDiskCommand?
Line 39:
enclosingCommand.getParameters().setImageGroupID(Guid.newGuid());
Line 40: enclosingCommand.getParameters().setEntityInfo(
Line 41: new EntityInfo(VdcObjectType.Disk,
enclosingCommand.getParameters().getImageGroupID()));
Line 42:
Line 56: diskImage.setVolumeType(VolumeType.Sparse);
Line 57: }
Line 58: } else {
Line 59: diskImage.setVolumeType(VolumeType.Sparse);
Line 60: }
consider change this to:
if (diskImage.getVolumeFormat() == VolumeFormat.RAW
&&getDestinationStorageDomain().getStorageType().isBlockDomain() ) {
diskImage.setVolumeType(VolumeType.Preallocated);
}
else {
diskImage.setVolumeType(VolumeType.Sparse);
}
Line 61:
Line 62: VdcReturnValueBase vdcReturnValue =
Line 63:
Backend.getInstance().runInternalAction(VdcActionType.AddImageFromScratch,
Line 64: getAddImageFromScratchParameters(),
ExecutionHandler.createInternalJobContext());
Line 100: return parameters;
Line 101: }
Line 102:
Line 103: @Override
Line 104: public void endSuccessfully() {
you should have here setSucceeded(true) IIRC.
Line 105: }
Line 106:
Line 107: @Override
Line 108: public void compensate() {
Line 135: return null;
Line 136: }
Line 137:
Line 138: @Override
Line 139: public void endWithFailure() {
audit log ? setSucceeded(true) should be added IIRC
Line 140: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IsoDomainListSyncronizer.java
Line 22: import
org.ovirt.engine.core.common.businessentities.StorageDomainStatus;
Line 23: import org.ovirt.engine.core.common.businessentities.StorageDomainType;
Line 24: import org.ovirt.engine.core.common.businessentities.StoragePoolIsoMap;
Line 25: import org.ovirt.engine.core.common.businessentities.StoragePoolStatus;
Line 26: import org.ovirt.engine.core.common.businessentities.StorageType;
seems like this is related to a different patch
Line 27: import org.ovirt.engine.core.common.businessentities.VDSStatus;
Line 28: import org.ovirt.engine.core.common.config.Config;
Line 29: import org.ovirt.engine.core.common.config.ConfigValues;
Line 30: import org.ovirt.engine.core.common.utils.Pair;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/OpenstackImageProviderProxy.java
Line 220: }
Line 221:
Line 222: private long getCowVirtualSize(String id) throws IOException {
Line 223: // For the qcow2 format we need to download the image header
and read the virtual size from there
Line 224: byte[] imgContent = new byte[72];
perhaps export the length to a constant
Line 225: ImageDownload downloadImage =
getGlanceClient().images().download(id).execute();
Line 226:
Line 227: try {
Line 228: int bytesRead =
downloadImage.getInputStream().read(imgContent, 0, imgContent.length);
--
To view, visit http://gerrit.ovirt.org/16082
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b3e9497b633bd2ad32b896aea0aaab80634f2a7
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
