Allon Mureinik has posted comments on this change. Change subject: [WIP] core: introduce RemoveSnapshotSingleDiskLive BLL command ......................................................................
Patch Set 5: (22 comments) http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DestroyImageCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DestroyImageCommand.java: Line 34: setCommandStatus(CommandStatus.ACTIVE_ASYNC); Line 35: Guid taskId = persistAsyncTaskPlaceHolder(VdcActionType.DestroyImage); Line 36: Line 37: VDSReturnValue vdsReturnValue = Backend.getInstance().getResourceManager() Line 38: .RunVdsCommand(VDSCommandType.DestroyImage, getVDSParameters()); s/Backend.getInstance().getResourceManager().RunVdsCommand/runVdsCommand Line 39: Line 40: if (vdsReturnValue != null && vdsReturnValue.getCreationInfo() != null) { Line 41: // TODO GP should we add a string message for createTask? Line 42: getReturnValue().getInternalVdsmTaskIdList().add( Line 51: setCommandStatus(CommandStatus.FAILED); Line 52: } Line 53: } Line 54: Line 55: private VDSParametersBase getVDSParameters() { I think createVDSParameters() would be a better name. Also, should be static. Line 56: return new DestroyImageVDSCommandParameters( Line 57: getParameters().getStoragePoolId(), Line 58: getParameters().getStorageDomainId(), Line 59: getParameters().getImageGroupId(), Line 56: return new DestroyImageVDSCommandParameters( Line 57: getParameters().getStoragePoolId(), Line 58: getParameters().getStorageDomainId(), Line 59: getParameters().getImageGroupId(), Line 60: // TODO GP no need to make a list from the list I don't get it Line 61: new ArrayList<Guid>(getParameters().getImageList()), Line 62: getParameters().isPostZero(), Line 63: getParameters().isForce()); Line 64: } Line 64: } Line 65: Line 66: @Override Line 67: public List<PermissionSubject> getPermissionCheckSubjects() { Line 68: return null; Yikes! AFAIK, this should be copied from the cold merge command Line 69: } Line 70: Line 71: @Override Line 72: public AsyncTaskType getTaskType() { Line 78: return new DestroyImageCommandCallback(); Line 79: } Line 80: Line 81: /* Line 82: TODO GP use for permission check subjects? This basically looks right Line 83: @Override Line 84: protected Guid[] getTaskObjects() { Line 85: Guid storageDomainId = !CollectionUtils.isEmpty(getEnclosingCommand().getDiskImage().getStorageIds()) ? Line 86: getEnclosingCommand().getDiskImage().getStorageIds().get(0) : Guid.Empty; http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DestroyImageCommandCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DestroyImageCommandCallback.java: Line 12: Line 13: public class DestroyImageCommandCallback extends CommandCallBack { Line 14: @Override Line 15: public void executed(VdcReturnValueBase result) { Line 16: return; huh? Line 17: } Line 18: Line 19: @Override Line 20: public void doPolling(Guid cmdId, List<Guid> childCmdIds) { Line 17: } Line 18: Line 19: @Override Line 20: public void doPolling(Guid cmdId, List<Guid> childCmdIds) { Line 21: // if (AsyncTaskManager.getInstance().) ? Line 22: DestroyImageCommand cmd = getCommand(cmdId); Line 23: AsyncTasks task = TaskManagerUtil.getAsyncTaskFromDb(cmd.getParameters().getVdsmTaskIds().get(0)); Line 24: if (task.getstatus() == AsyncTaskStatusEnum.finished) { Line 25: cmd.setCommandStatus(task.getCommandStatus()); Line 23: AsyncTasks task = TaskManagerUtil.getAsyncTaskFromDb(cmd.getParameters().getVdsmTaskIds().get(0)); Line 24: if (task.getstatus() == AsyncTaskStatusEnum.finished) { Line 25: cmd.setCommandStatus(task.getCommandStatus()); Line 26: // TODO GP ^^^ or below? Line 27: //cmd.setCommandStatus(task.getresult() == AsyncTaskResultEnum.success /*(or cleanSuccess?)*/ ? CommandStatus.SUCCEEDED : CommandStatus.FAILED); This one looks better, IMHO Line 28: } Line 29: } Line 30: Line 31: @Override Line 29: } Line 30: Line 31: @Override Line 32: public void onFailed(Guid cmdId, List<Guid> childCmdIds) { Line 33: return; ? Line 34: } Line 35: Line 36: @Override Line 37: public void onSucceeded(Guid cmdId, List<Guid> childCmdIds) { Line 34: } Line 35: Line 36: @Override Line 37: public void onSucceeded(Guid cmdId, List<Guid> childCmdIds) { Line 38: return; ? Line 39: } Line 40: Line 41: private DestroyImageCommand<DestroyImageParameters> getCommand(Guid cmdId) { Line 42: return (DestroyImageCommand<DestroyImageParameters>) TaskManagerUtil.retrieveCommand(cmdId); http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeCommand.java: Line 89: getEnclosingCommand().getDiskImage().getStorageIds().get(0) : Guid.Empty; Line 90: Line 91: return new Guid[] { storageDomainId }; Line 92: } Line 93: */ ? http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeCommandCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeCommandCallback.java: Line 17: private static final Log log = LogFactory.getLog(MergeCommandCallback.class); Line 18: Line 19: @Override Line 20: public void executed(VdcReturnValueBase result) { Line 21: return; ? Line 22: } Line 23: Line 24: @Override Line 25: public void doPolling(Guid cmdId, List<Guid> childCmdIds) { Line 25: public void doPolling(Guid cmdId, List<Guid> childCmdIds) { Line 26: // If the VM Job exists, the command is still active Line 27: boolean isRunning = false; Line 28: MergeCommand<MergeParameters> command = getCommand(cmdId); Line 29: List<VmJob> vmJobs = DbFacade.getInstance().getVmJobDao().getAllForVmDisk( s/DbFacade.getInstance()/getDbFacade()/ Line 30: command.getParameters().getVmId(), Line 31: command.getParameters().getImageGroupId()); Line 32: for (VmJob vmJob : vmJobs) { Line 33: if (vmJob.getId().equals(command.getParameters().getVmJobId())) { http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeStatusCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeStatusCommand.java: Line 88: } Line 89: Line 90: @Override Line 91: public List<PermissionSubject> getPermissionCheckSubjects() { Line 92: // TODO GP any? It's an internal command - the permission list is meaningless. Line 93: return null; Line 94: } Line 95: Line 96: @Override http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeStatusCommandCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeStatusCommandCallback.java: Line 30: MergeStatusCommand<MergeParameters> command = getCommand(cmdId); Line 31: Set<Guid> images = command.getParameters().getVmVolumeChain(); Line 32: Line 33: if (images == null) { Line 34: return; Perhaps add a log here? Line 35: } Line 36: Line 37: // Our contract with vdsm merge states that if the VM is found down, we Line 38: // have to assume the merge failed. (It's okay if it really succeeded, http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskCommandBase.java: Line 52: @Override Line 53: protected void endWithFailure() { Line 54: // TODO: FILL! We should determine what to do in case of Line 55: // failure (is everything rolled-backed? rolled-forward? Line 56: // some and some?). You can't roll back a failed snapshot. My two cents - raise an audit log, and let the user retry when we wants/ Line 57: setSucceeded(true); Line 58: } Line 59: Line 60: private String getSnapshotDescriptionById(Guid snapshotId) { http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskLiveCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskLiveCommand.java: Line 81: // Finish MergeStatus, start DestroyImage Line 82: getParameters().setMergeStatusReturnValue((MergeStatusReturnValue) vdcReturnValue.getActionReturnValue()); Line 83: future = TaskManagerUtil.executeAsyncCommand(VdcActionType.DestroyImage, buildDestroyImageParameters()); Line 84: break; Line 85: case 3: Can we get constants for these steps? Just to make it clear what we're doing in each step? Line 86: // Finish DestroyImage Line 87: getParameters().setDestroyImageCommandComplete(true); Line 88: setCommandStatus(CommandStatus.SUCCEEDED); Line 89: break; Line 163: } Line 164: Line 165: @Override Line 166: protected void endSuccessfully() { Line 167: // TODO GP call super? Probably. Line 168: } Line 169: Line 170: public void onFailed() { Line 171: endWithFailure(); Line 182: return new RemoveSnapshotSingleDiskLiveCommandCallback(); Line 183: } Line 184: Line 185: Line 186: // TODO GP how does compensation work with the CommandExecutor? Removing a snapshot can't compensate - you started the process of removing something. There's no way of bringing it back. Line 187: /* Line 188: @Override Line 189: public void preventRollback() { Line 190: getParameters().setExecutionIndex(0); http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveSnapshotSingleDiskParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveSnapshotSingleDiskParameters.java: Line 77: Line 78: public void setBlockJobType(VmBlockJobType blockJobType) { Line 79: this.blockJobType = blockJobType; Line 80: } Line 81: */ ? Line 82: Line 83: public boolean isDestroyImageCommandComplete() { Line 84: return destroyImageCommandComplete; Line 85: } http://gerrit.ovirt.org/#/c/26909/5/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 147: RemoveVmHibernationVolumes(233, QuotaDependency.NONE), Line 148: RemoveMemoryVolumes(234, QuotaDependency.NONE), Line 149: RemoveDiskSnapshots(235, ActionGroup.MANIPULATE_VM_SNAPSHOTS, QuotaDependency.NONE), Line 150: RemoveSnapshotSingleDiskLive(236, QuotaDependency.STORAGE), Line 151: // TODO GP check QuotaDependency Seems though they should all be Storage Line 152: Merge(237, QuotaDependency.STORAGE), Line 153: MergeStatus(238, QuotaDependency.NONE), Line 154: DestroyImage(239, QuotaDependency.STORAGE), Line 155: // VmPoolCommands http://gerrit.ovirt.org/#/c/26909/5/backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties: Line 407: UPDATE_VNIC_FAILED=Failed to update VM Network Interface. Line 408: VolumeResizeValueError=Incorrect size value for volume resize Line 409: ResizeErr=Wrong resize disk parameter Line 410: UpdateDevice=Failed to update device Line 411: SETUP_NETWORKS_ROLLBACK=Reverting back to last known saved configuration. rebase foobar? please remove. -- To view, visit http://gerrit.ovirt.org/26909 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic47eb91a0ea1fe150e3b2152e2c9d5f1f2eb3678 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
