Liron Aravot has posted comments on this change.
Change subject: core: WIP : RemoveImageDisk - race when updating snapshots ovf
(#828192)
......................................................................
Patch Set 14: (6 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java
Line 99: getCommandId()));
Line 100: p.setEntityId(getParameters().getEntityId());
Line 101: return
AsyncTaskManager.getInstance().CreateTask(AsyncTaskType.deleteImage, p);
Line 102: }
Line 103:
when performing remove all vm images command we should not perform any ovf
update at all which happens at the moment. this will be solved in a further
patch as it doesn't relate to this race condition fix.
Line 104: private void removeImageFromDB() {
Line 105: final DiskImage diskImage = getDiskImage();
Line 106: final List<Snapshot> updatedSnapshots;
Line 107: final boolean shouldPerformOpOnVmSnapshots =
!diskImage.isShareable();
Line 106: final List<Snapshot> updatedSnapshots;
Line 107: final boolean shouldPerformOpOnVmSnapshots =
!diskImage.isShareable();
Line 108:
Line 109: try {
Line 110: if (shouldPerformOpOnVmSnapshots) {
readability and for not repeating this line few times in that code (same
variable used here few times).
Line 111: VM vm = getVmForNonShareableDiskImage(diskImage);
Line 112: lockVmSnapshotsWithWait(vm);
Line 113: updatedSnapshots =
prepareSnapshotConfigWithoutImage(diskImage.getId());
Line 114: } else {
Line 110: if (shouldPerformOpOnVmSnapshots) {
Line 111: VM vm = getVmForNonShareableDiskImage(diskImage);
Line 112: lockVmSnapshotsWithWait(vm);
Line 113: updatedSnapshots =
prepareSnapshotConfigWithoutImage(diskImage.getId());
Line 114: } else {
that's the existing logic, i agree that it might be unnecessary and may be
examined and fixed in a further patch.
Line 115: updatedSnapshots = Collections.emptyList();
Line 116: }
Line 117:
Line 118:
TransactionSupport.executeInScope(TransactionScopeOption.Required,
Line 150:
Line 151: return null;
Line 152: }
Line 153: });
Line 154: }
Done
Line 155: finally {
Line 156: if (shouldPerformOpOnVmSnapshots) {
Line 157: freeVmSnapshotsLock();
Line 158: }
Line 157: freeVmSnapshotsLock();
Line 158: }
Line 159: }
Line 160: }
Line 161:
will be moved up,
it's created by default because in case of a failure we attempt to perform
unlock in the finally block - so it's either create it in the method and
initiate it there and send it as a parameter to the lock/unlock functions which
is unrelated to the method purpose or create it as a member which is better
'looking' in my opinion and will allow us to use it also outside of this method
scope. creating it inside the business method will be to mix locking related
logic with the business logic and i prefer to separate it.
Line 162: private final EngineLock snapshotsEngineLock = new EngineLock();
Line 163:
Line 164: private void lockVmSnapshotsWithWait(VM vm) {
Line 165: Map<String, String> snapshotsExlusiveLockMap =
Collections.singletonMap(vm.getId().toString(),
LockingGroup.VM_SNAPSHOTS.name());
Line 165: Map<String, String> snapshotsExlusiveLockMap =
Collections.singletonMap(vm.getId().toString(),
LockingGroup.VM_SNAPSHOTS.name());
Line 166:
snapshotsEngineLock.setExclusiveLocks(snapshotsExlusiveLockMap);
Line 167: getLockManager().acquireLockWait(snapshotsEngineLock);
Line 168: }
Line 169:
better readability in my opinion.
Line 170: private void freeVmSnapshotsLock(){
Line 171: getLockManager().releaseLock(snapshotsEngineLock);
Line 172: }
Line 173:
--
To view, visit http://gerrit.ovirt.org/7482
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iccb44f1aa9d204477955343167133849a4146753
Gerrit-PatchSet: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
