Michael Kublin has uploaded a new change for review. Change subject: engine: Fixing RecoveryStoragePool and EventQueue ......................................................................
engine: Fixing RecoveryStoragePool and EventQueue The following fix is doing the following: 1. The event trigerred by RecoveryStoragePoll will be cleaned from queue if a previous event was reconstruct and it finished with success 2. The event of RecoveryStoragePool will be added the first to the queue, because of it was triggered by user 3. Removed a code from RecoveryStoragePool which should prevent a race, the reason is no race is prevented, code was duplicated, the logic was duplicated, the logic should be simple and rewritted in other way Change-Id: I3b58f095c69c76116f0af44b08ccaf41d1ff1fb8 Signed-off-by: Michael Kublin <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/eventqueue/EventQueueMonitor.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RecoveryStoragePoolCommand.java 2 files changed, 39 insertions(+), 36 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/01/11201/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/eventqueue/EventQueueMonitor.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/eventqueue/EventQueueMonitor.java index 39e1704..4f8b5e1 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/eventqueue/EventQueueMonitor.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/eventqueue/EventQueueMonitor.java @@ -3,7 +3,6 @@ import java.util.HashMap; import java.util.LinkedList; import java.util.Map; -import java.util.Queue; import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -37,8 +36,8 @@ private static Log log = LogFactory.getLog(EventQueueMonitor.class); private static final ConcurrentMap<Guid, ReentrantLock> poolsLockMap = new ConcurrentHashMap<Guid, ReentrantLock>(); - private static final Map<Guid, Queue<Pair<Event, FutureTask<EventResult>>>> poolsEventsMap = - new HashMap<Guid, Queue<Pair<Event, FutureTask<EventResult>>>>(); + private static final Map<Guid, LinkedList<Pair<Event, FutureTask<EventResult>>>> poolsEventsMap = + new HashMap<Guid, LinkedList<Pair<Event, FutureTask<EventResult>>>>(); private static final Map<Guid, Event> poolCurrentEventMap = new HashMap<Guid, Event>(); @Override @@ -72,18 +71,18 @@ switch (currentEvent.getEventType()) { case RECONSTRUCT: if (event.getEventType() == EventType.VDSCONNECTTOPOOL || event.getEventType() == EventType.RECOVERY) { - task = addTaskToQueue(event, callable, storagePoolId); + task = addTaskToQueue(event, callable, storagePoolId, isEventShouldBeFirst(event)); } else { log.debugFormat("Current event was skiped because of reconstruct is running now for pool {0}, event {1}", storagePoolId, event); } break; default: - task = addTaskToQueue(event, callable, storagePoolId); + task = addTaskToQueue(event, callable, storagePoolId, isEventShouldBeFirst(event)); break; } } else { - task = addTaskToQueue(event, callable, storagePoolId); + task = addTaskToQueue(event, callable, storagePoolId, false); poolCurrentEventMap.put(storagePoolId, event); ThreadPoolUtil.execute(new InternalEventQueueThread(storagePoolId, lock, poolsEventsMap, poolCurrentEventMap)); @@ -94,14 +93,30 @@ return task; } - private FutureTask<EventResult> addTaskToQueue(Event event, Callable<EventResult> callable, Guid storagePoolId) { + /** + * The following method should decide if we want that the event will be first for executing, before all other events + * already submitted to queue + * @param event + * - submitted event + * @return + */ + private boolean isEventShouldBeFirst(Event event) { + return event.getEventType() == EventType.RECOVERY; + } + + private FutureTask<EventResult> addTaskToQueue(Event event, Callable<EventResult> callable, Guid storagePoolId, boolean addFirst) { FutureTask<EventResult> task = new FutureTask<EventResult>(callable); - getEventQueue(storagePoolId).add(new Pair<Event, FutureTask<EventResult>>(event, task)); + Pair<Event, FutureTask<EventResult>> queueEvent = new Pair<Event, FutureTask<EventResult>>(event, task); + if (addFirst) { + getEventQueue(storagePoolId).addFirst(queueEvent); + } else { + getEventQueue(storagePoolId).add(queueEvent); + } return task; } - private Queue<Pair<Event, FutureTask<EventResult>>> getEventQueue(Guid storagePoolId) { - Queue<Pair<Event, FutureTask<EventResult>>> queue = poolsEventsMap.get(storagePoolId); + private LinkedList<Pair<Event, FutureTask<EventResult>>> getEventQueue(Guid storagePoolId) { + LinkedList<Pair<Event, FutureTask<EventResult>>> queue = poolsEventsMap.get(storagePoolId); if (queue == null) { queue = new LinkedList<Pair<Event, FutureTask<EventResult>>>(); poolsEventsMap.put(storagePoolId, queue); @@ -121,11 +136,11 @@ private Guid storagePoolId; private ReentrantLock lock; private Map<Guid, Event> poolCurrentEventMap; - private Map<Guid, Queue<Pair<Event, FutureTask<EventResult>>>> poolsEventsMap; + private Map<Guid, LinkedList<Pair<Event, FutureTask<EventResult>>>> poolsEventsMap; public InternalEventQueueThread(Guid storagePoolId, ReentrantLock lock, - Map<Guid, Queue<Pair<Event, FutureTask<EventResult>>>> poolsEventsMap, + Map<Guid, LinkedList<Pair<Event, FutureTask<EventResult>>>> poolsEventsMap, Map<Guid, Event> poolCurrentEventMap) { this.storagePoolId = storagePoolId; this.lock = lock; @@ -159,11 +174,11 @@ log.infoFormat("Finished reconstruct for pool {0}. Clearing event queue", storagePoolId); lock.lock(); try { - Queue<Pair<Event, FutureTask<EventResult>>> queue = + LinkedList<Pair<Event, FutureTask<EventResult>>> queue = new LinkedList<Pair<Event, FutureTask<EventResult>>>(); for (Pair<Event, FutureTask<EventResult>> task : poolsEventsMap.get(storagePoolId)) { EventType eventType = task.getFirst().getEventType(); - if (eventType == EventType.VDSCONNECTTOPOOL || eventType == EventType.RECOVERY) { + if (eventType == EventType.VDSCONNECTTOPOOL || (eventType == EventType.RECOVERY && !result.isSuccess())) { queue.add(task); } else { log.infoFormat("The following operation {0} was cancelled, because of recosntruct was run before", diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RecoveryStoragePoolCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RecoveryStoragePoolCommand.java index d95d0e9..a9296f9 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RecoveryStoragePoolCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RecoveryStoragePoolCommand.java @@ -104,29 +104,17 @@ new Callable<EventResult>() { @Override public EventResult call() { - // set those to null in order to reload them during canDoAction - - // canDo checks should be performed on updated values and not staled ones. - // canDoAction checks are needed here as Recovery operations aren't cleared from the event queue - // after reconstruct, in order to provide the user ability to recover from the state of non working pool - // without him to be in a race with automatic triggered reconstruct. because of that, we don't want to run - // the recovery operation if it's unneeded so we need to re-check the canDoAction result. canDoAction() method was - // as is in order to provide the user immediate response whether it's possible to initiate the command when - // he attempts to run recovery. - setStorageDomain(null); - setStoragePool(null); - if (canDoAction()) { - StoragePoolIsoMap domainPoolMap = - new StoragePoolIsoMap(getRecoveryStoragePoolParametersData() - .getNewMasterDomainId(), - getRecoveryStoragePoolParametersData().getStoragePoolId(), - StorageDomainStatus.Active); - DbFacade.getInstance() - .getStoragePoolIsoMapDao() - .save(domainPoolMap); + StoragePoolIsoMap domainPoolMap = + new StoragePoolIsoMap(getRecoveryStoragePoolParametersData() + .getNewMasterDomainId(), + getRecoveryStoragePoolParametersData().getStoragePoolId(), + StorageDomainStatus.Active); + DbFacade.getInstance() + .getStoragePoolIsoMapDao() + .save(domainPoolMap); - getStoragePool().setstatus(StoragePoolStatus.Problematic); - executeReconstruct(); - } + getStoragePool().setstatus(StoragePoolStatus.Problematic); + executeReconstruct(); return new EventResult(reconstructOpSucceeded, EventType.RECONSTRUCT); } }); -- To view, visit http://gerrit.ovirt.org/11201 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3b58f095c69c76116f0af44b08ccaf41d1ff1fb8 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Michael Kublin <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
