Greg Padgett has posted comments on this change. Change subject: core: introduce RemoveSnapshotSingleDiskLive BLL command ......................................................................
Patch Set 5: (18 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 Done 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. Name change done. I'd have to pass in the parameters to make the method 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 Just some cleanup. In order to fix it, DestroyImageVDSCommandParameters' imageList needed to be changed from ArrayList to List. 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 It's internal, but I added the domain anyway as this is used also for the list of affected objects. Line 69: } Line 70: Line 71: @Override Line 72: public AsyncTaskType getTaskType() { 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? No need for these with Ravi's latest changes 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()); > For Async tasks the command status should automatically be set by AsyncTask Done Line 26: // TODO GP ^^^ or below? Line 27: //cmd.setCommandStatus(task.getresult() == AsyncTaskResultEnum.success /*(or cleanSuccess?)*/ ? CommandStatus.SUCCEEDED : CommandStatus.FAILED); Line 28: } Line 29: } 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 Done Line 28: } Line 29: } Line 30: Line 31: @Override 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; > I will handle the above at CommandCallBack to provide default empty impleme Thanks 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()/ No such convenience method here :) 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())) { 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())) { Line 34: isRunning = true; > I think you can break out of the loop here. Indeed, thanks Line 35: } Line 36: } Line 37: Line 38: if (!isRunning) { 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. See comment on DestroyImageCommand 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? Done 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. I think it makes sense to do this, but since I haven't changed this code (only moved it from RemoveSnapshotSingleDiskCommand.java), let's improve it in a subsequent patch. 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 doin Done 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. Looked into it, RemoveSnapshotSingleDiskCommand doesn't call its superclass constructor. It seems our ancestor BaseImagesCommand.endSuccessfully() does some things we don't want to do when merging snapshots. 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 Mainly wanted to make sure any compensation attempts would mess things up, i.e. if I needed something like the below preventRollback() 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/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskLiveCommandCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskLiveCommandCallback.java: Line 16: } Line 17: Line 18: @Override Line 19: public void doPolling(Guid cmdId, List<Guid> childCmdIds) { Line 20: getCommand(cmdId).proceedCommandExecution(); > You do the proceedCommandExecution logic every time doPolling is called. No problems, the function was written to behave sanely no matter the state. it will either be a no-op (step not complete), step forward (step succeeded), or set the end state to stop the polling (failure, or everything done). Line 21: } Line 22: Line 23: @Override Line 24: public void onFailed(Guid cmdId, List<Guid> childCmdIds) { 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. Indeed, done -- 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
