Liron Ar has posted comments on this change.
Change subject: wip: core: RemoveImage as sync operation
......................................................................
Patch Set 4: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyImageGroupCommand.java
Line 190:
Line 191: @Override
Line 192: protected void revertTasks() {
Line 193: // Revert should be performed only for AddVmFromSnapshot at
this point.
Line 194: if (getParameters().getParentCommand() ==
VdcActionType.AddVmFromSnapshot || getParameters().getParentCommand() ==
VdcActionType.ImportVm) {
i refactored this whole method already in another patch here : i'll rebase it
on top of that one once this one get merged -
http://gerrit.ovirt.org/#/c/12689/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyImageGroupCommand.java
Line 195: Guid destImageId =
getParameters().getDestinationImageId();
Line 196: RemoveImageParameters removeImageParams =
Line 197: new RemoveImageParameters(destImageId);
Line 198: if (getParameters().getParentCommand() ==
VdcActionType.ImportVm) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java
Line 259: DiskImage diskImage = getDiskImage();
Line 260: RemoveImageParameters p = new
RemoveImageParameters(diskImage.getImageId());
Line 261:
p.setTransactionScopeOption(TransactionScopeOption.Suppress);
Line 262: p.setDiskImage(diskImage);
Line 263: p.setParentCommand(VdcActionType.RemoveDisk);
it was already removed, take a look in the parameters class.
Line 264: p.setEntityId(getParameters().getEntityId());
Line 265: p.setParentParameters(getParameters());
Line 266: p.setRemoveFromSnapshots(true);
Line 267:
p.setStorageDomainId(getParameters().getStorageDomainId());
Line 312: }
Line 313:
Line 314: @Override
Line 315: public AuditLogType getAuditLogTypeValue() {
Line 316: switch (getActionState()) {
if you don't mind i prefer to leave it like that.
Line 317: case EXECUTE:
Line 318: return getSucceeded() ?
AuditLogType.USER_FINISHED_REMOVE_DISK
Line 319: : AuditLogType.USER_FINISHED_FAILED_REMOVE_DISK;
Line 320: default:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java
Line 85: getParameters().getParentCommand(),
Line 86: VdcObjectType.Storage,
Line 87: getParameters().getStorageDomainId()));
Line 88: } catch (VdcBLLException e) {
Line 89: if (e.getErrorCode() !=
VdcBllErrors.ImageDoesNotExistInDomainError) {
ok, i'll add a log in DEBUG
Line 90: throw e;
Line 91: }
Line 92: }
Line 93:
Line 112:
Line 113: try {
Line 114: VM vm = getVmForNonShareableDiskImage(diskImage);
Line 115: // if the disk is not part of a vm (floating), there are
no snapshots to update
Line 116: // so no lock is required.
why not?
Line 117: if (getParameters().isRemoveFromSnapshots() && vm !=
null) {
Line 118: lockVmSnapshotsWithWait(vm);
Line 119: updatedSnapshots =
prepareSnapshotConfigWithoutImage(diskImage.getId());
Line 120: } else {
Line 117: if (getParameters().isRemoveFromSnapshots() && vm !=
null) {
Line 118: lockVmSnapshotsWithWait(vm);
Line 119: updatedSnapshots =
prepareSnapshotConfigWithoutImage(diskImage.getId());
Line 120: } else {
Line 121: updatedSnapshots = Collections.emptyList();
hi, those changes can be done in a further patch - they aren't related to the
bug (not that i'm against further refactor here, but i want to keep the changes
here to minimal).
Line 122: }
Line 123:
Line 124:
TransactionSupport.executeInScope(TransactionScopeOption.Required,
Line 125: new TransactionMethod<Object>() {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java
Line 230: public AuditLogType getAuditLogTypeValue() {
Line 231: switch (getActionState()) {
Line 232: case EXECUTE:
Line 233: return getSucceeded() ?
AuditLogType.USER_REMOVE_VM_FINISHED : (hasImages ? AuditLogType.
Line 234: USER_REMOVE_VM_FINISHED_WITH_ILLEGAL_DISKS :
AuditLogType.USER_FAILED_REMOVE_VM);
why? if getSucceeded() we print success message, only if it failed we check if
there are images to show correct message.
Line 235: case END_FAILURE:
Line 236: case END_SUCCESS:
Line 237: default:
Line 238: return disksLeftInVm.isEmpty() ?
AuditLogType.USER_REMOVE_VM_FINISHED
Line 234: USER_REMOVE_VM_FINISHED_WITH_ILLEGAL_DISKS :
AuditLogType.USER_FAILED_REMOVE_VM);
Line 235: case END_FAILURE:
Line 236: case END_SUCCESS:
Line 237: default:
Line 238: return disksLeftInVm.isEmpty() ?
AuditLogType.USER_REMOVE_VM_FINISHED
Done
Line 239: :
AuditLogType.USER_REMOVE_VM_FINISHED_WITH_ILLEGAL_DISKS;
Line 240: }
Line 241: }
Line 242:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateFromImportExportCommand.java
Line 128: }
Line 129:
Line 130: @Override
Line 131: public AuditLogType getAuditLogTypeValue() {
Line 132: switch (getActionState()) {
I prefer to leave it if that's fine by you.
Line 133: case EXECUTE:
Line 134: return getSucceeded() ?
AuditLogType.IMPORTEXPORT_REMOVE_TEMPLATE
Line 135: :
AuditLogType.IMPORTEXPORT_REMOVE_TEMPLATE_FAILED;
Line 136: default:
--
To view, visit http://gerrit.ovirt.org/13611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic822a92acb8231d2ca84ae092bd98659507925d8
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
