Arik Hadas has posted comments on this change.
Change subject: core: LSM - lock changes, added support for disk snapshot
scenarios
......................................................................
Patch Set 5: Code-Review-1
(5 comments)
-1 because I remember that it is problematic to lock resources through the
whole LSM process with locks in LiveMigrateVmDisks command. Liron, please check
that its locks are released after all the LiveMigrateDisk commands end
....................................................
Commit Message
Line 9: This patch contains the following changes:
Line 10: *When live migrating a disk, the disk (and it's snapshots) would be
Line 11: locked for the whole flow preventing other operations to be done on the
Line 12: disk. Therefore the create snapshot command will ignore those disks in
Line 13: it's checks and they won't be unlocked as the snapshot phase ends.
it's -> its
Line 14:
Line 15: *Disk can be live migrated when snapshots of it are attached to other
Line 16: vms unless those vms are not in "Down" status as it means that the disk
Line 17: is being used by them.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveDisksCommand.java
Line 170: moveDiskParameters.getSourceDomainId(),
Line 171: moveDiskParameters.getStorageDomainId(),
Line 172: vmId,
Line 173: moveDiskParameters.getQuotaId());
Line 174:
toReturn.setImageGroupID(diskMap.get(moveDiskParameters.getImageId()).getId());
why not to add it to the parameters of LiveMigrateDiskParameters's constructor
to enforce that the parameters of the live migrate disk command will always
have the image group id?
Line 175: return toReturn;
Line 176: }
Line 177:
Line 178: private LiveMigrateVmDisksParameters
createLiveMigrateVmDisksParameters(List<MoveDiskParameters> moveDiskParamsList,
Guid vmId) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateDiskCommand.java
Line 185: return AuditLogType.UNASSIGNED;
Line 186: }
Line 187:
Line 188: @Override
Line 189: protected Map<String, Pair<String, String>> getExclusiveLocks() {
I think it is problematic because you want the disks to be locked for the
entire LSM process, but when you lock it in LiveMigrateVmDisks command, even
for its entire execution, the locks will be freed before all the
LiveMigrateDisk commands are finished.
Line 190: return null;
Line 191: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveSnapshotTaskHandler.java
Line 23:
Line 24: public class LiveSnapshotTaskHandler implements SPMAsyncTaskHandler {
Line 25:
Line 26: private final TaskHandlerCommand<? extends
LiveMigrateVmDisksParameters> enclosingCommand;
Line 27: private LinkedHashSet<Guid> movedVmDiskIds = null;
it will be null by default, no need to explicitly initialize it to null
Line 28:
Line 29: public LiveSnapshotTaskHandler(TaskHandlerCommand<? extends
LiveMigrateVmDisksParameters> enclosingCommand) {
Line 30: this.enclosingCommand = enclosingCommand;
Line 31: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ImagesActionsParametersBase.java
Line 99: public void setImportEntity(boolean importEntity) {
Line 100: this.importEntity = importEntity;
Line 101: }
Line 102:
Line 103: public boolean isShouldLeaveLocked() {
I also think that it should be changed, but I think that changing it to should
and keep setShouldLeaveLocked method might confuse JSON (I'm not sure about it
though). I would prefer to have isLeaveLocked & setLeaveLocked to conform the
conventions
Line 104: return shouldLeaveLocked;
Line 105: }
Line 106:
Line 107: public void setShouldLeaveLocked(boolean shouldLeaveLocked) {
--
To view, visit http://gerrit.ovirt.org/20847
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c7cb49ff37f28d0b3ab0df9f4ee1f3082aba018
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
