Allon Mureinik has posted comments on this change.
Change subject: core: prevent simultaneous ReconstructsMasterDomain on the same
pool(#845838)
......................................................................
Patch Set 9: I would prefer that you didn't submit this
(7 inline comments)
Ayal - please take a look at the apperror* files.
Is the use of the term Storage Pool OK there?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ReconstructMasterDomainCommand.java
Line 59: // in order to avoid interrupting automatic processes.
Line 60: if (VdcActionType.RecoveryStoragePool.equals(getActionType())
&&
Line 61:
!IrsBrokerCommand.tryLockIrsProxyData(getStoragePool().getId())) {
Line 62:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_DURING_RECONSTRUCT);
Line 63: return false;
addCanDoActionMessage + return false = failCanDoAction
Line 64: } else {
Line 65: // otherwise we perform ReconstructMasterDomain command
and should wait until we lock the IrsProxyData
Line 66: // of the pool.
Line 67:
IrsBrokerCommand.lockIrsProxyData(getStoragePool().getId());
Line 65: // otherwise we perform ReconstructMasterDomain command
and should wait until we lock the IrsProxyData
Line 66: // of the pool.
Line 67:
IrsBrokerCommand.lockIrsProxyData(getStoragePool().getId());
Line 68: }
Line 69:
Consider extracting the locking block to a method - IMHO, it'll be more
readable.
Line 70: try {
Line 71: toReturn = performChecks();
Line 72: } catch (Exception e) {
Line 73: toReturn = false;
Line 71: toReturn = performChecks();
Line 72: } catch (Exception e) {
Line 73: toReturn = false;
Line 74: } finally {
Line 75:
IrsBrokerCommand.unLockIrsProxyData(getStoragePool().getId());
this is done at the end of the execute() - shouldn't it be remove?
Line 76: }
Line 77: return toReturn;
Line 78: }
Line 79:
Line 184: .getValue(),
ReconstructMarkAction.ClearCache));
Line 185: }
Line 186: // if we got here - it mean that the IrsProxyData lock was
performed by
Line 187: // canDoAction() method and that it's still held by this
thread.
Line 188: IrsBrokerCommand.unLockIrsProxyData(getStoragePool().getId());
this should be inside the finally block
Line 189: }
Line 190:
Line 191: protected boolean stopSpm() {
Line 192: boolean commandSucceeded = true;
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java
Line 1553: irsProxyData.clearVdsFromCache(vdsId, vdsName);
Line 1554: }
Line 1555: }
Line 1556:
Line 1557: public static boolean tryLockIrsProxyData(Guid storagePoolId) {
align the null treatment in all three methods
Line 1558: // implemented this way to prevent NPE in case that the pool
is removed from the map.
Line 1559: // TODO: prevent such a case when running DestoryStoragePool
Line 1560: try {
Line 1561: return
_irsProxyData.get(storagePoolId).tryLockIrsProxyLockObject();
....................................................
Commit Message
Line 4: Commit: Liron Aravot <[email protected]>
Line 5: CommitDate: 2012-09-27 17:42:52 +0200
Line 6:
Line 7: core: prevent simultaneous ReconstructsMasterDomain on the same
Line 8: pool(#845838)
make this fit in the 50 chars.
ReconstructsMasterDomain -> reconstruct should do the trick
Line 9:
Line 10: https://bugzilla.redhat.com/845838
Line 11:
Line 12: ReconsturctMasterDomain command can be triggered from the GUI (by
Line 15: simultanously, this patch prevents it by acquiring lock on the
Line 16: IrsProxyData object which represents the pool while executing the
Line 17: ReconstructMasterDomain command.
Line 18:
Line 19: Change-Id: Ia0dfb227b014af602e5bd1ab952d7c543992aa0f
please add Bug-Url:, as per the new template
--
To view, visit http://gerrit.ovirt.org/7982
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0dfb227b014af602e5bd1ab952d7c543992aa0f
Gerrit-PatchSet: 9
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 Paikov <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
