Hello Liron Aravot,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/31274
to review the following change.
Change subject: core: SyncLunsInfo - avoidable executions/db deadlock
......................................................................
core: SyncLunsInfo - avoidable executions/db deadlock
Currently SyncLunsInfoForBlockStorageDomainCommand is being executed
when connecting to a domain storage server. This command is resposible
to check if any of the luns of the domain were changed and in case it
did, update the DB accordingly.
When a storage domain is being activated all of it's hosts are being
connected simoltenously to the domain, which leads that
SyncLunsInfoForBlockStorageDomainCommand is executed multiple times
(once for each host) which is unneeded as it's exactly the same as
having response from one host (even if the responses of the hosts are
different, we can't decide which response is correct).
When being executed simoltenously, possibly concurrent updates from the
different threads can be made - as the order of the updates isn't
garuanteed, there's a chance for a deadlock.
This patch solves it by executes the operation for only one hosts at a
time, only in case of failure (which should be rare) the engine will
attempt to sync the luns through a different host.
Change-Id: I725964aa51ac38bf16d83cc498c5298720031632
Related-To-Bug-Url: https://bugzilla.redhat.com/1115845
Signed-off-by: Liron Aravot <lara...@redhat.com>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ActivateStorageDomainCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/FCPStorageHelper.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/IStorageHelper.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHelperBase.java
5 files changed, 40 insertions(+), 15 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/74/31274/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ActivateStorageDomainCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ActivateStorageDomainCommand.java
index 4a206f0..924ee41 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ActivateStorageDomainCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ActivateStorageDomainCommand.java
@@ -25,12 +25,12 @@
import org.ovirt.engine.core.common.businessentities.StoragePoolIsoMap;
import org.ovirt.engine.core.common.businessentities.StoragePoolIsoMapId;
import org.ovirt.engine.core.common.businessentities.StoragePoolStatus;
+import org.ovirt.engine.core.common.errors.VdcBllMessages;
import org.ovirt.engine.core.common.eventqueue.Event;
import org.ovirt.engine.core.common.eventqueue.EventQueue;
import org.ovirt.engine.core.common.eventqueue.EventResult;
import org.ovirt.engine.core.common.eventqueue.EventType;
import org.ovirt.engine.core.common.locks.LockingGroup;
-import org.ovirt.engine.core.common.errors.VdcBllMessages;
import org.ovirt.engine.core.common.utils.Pair;
import
org.ovirt.engine.core.common.vdscommands.ActivateStorageDomainVDSCommandParameters;
import org.ovirt.engine.core.common.vdscommands.VDSCommandType;
@@ -81,6 +81,18 @@
return returnValue;
}
+ private void syncStorageDomainInfo(List<Pair<Guid, Boolean>>
hostConnectionInfo) {
+ for (Pair<Guid, Boolean> pair : hostConnectionInfo) {
+ if (Boolean.TRUE.equals(pair.getSecond())) {
+ if (StorageHelperDirector.getInstance()
+ .getItem(getStorageDomain().getStorageType())
+ .syncDomainInfo(getStorageDomain(), pair.getFirst())) {
+ break;
+ }
+ }
+ }
+ }
+
@Override
protected void executeCommand() {
final StoragePoolIsoMap map =
@@ -95,7 +107,9 @@
freeLock();
log.infoFormat("ActivateStorage Domain. Before Connect all hosts to
pool. Time:{0}", new Date());
- connectHostsInUpToDomainStorageServer();
+ List<Pair<Guid, Boolean>> hostsConnectionResults =
connectHostsInUpToDomainStorageServer();
+ syncStorageDomainInfo(hostsConnectionResults);
+
runVdsCommand(VDSCommandType.ActivateStorageDomain,
new
ActivateStorageDomainVDSCommandParameters(getStoragePool().getId(),
getStorageDomain().getId()));
log.infoFormat("ActivateStorage Domain. After Connect all hosts to
pool. Time:{0}", new Date());
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/FCPStorageHelper.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/FCPStorageHelper.java
index da85123..36b1df1 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/FCPStorageHelper.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/FCPStorageHelper.java
@@ -3,8 +3,8 @@
import org.ovirt.engine.core.bll.Backend;
import org.ovirt.engine.core.common.action.StorageDomainParametersBase;
import org.ovirt.engine.core.common.action.VdcActionType;
-import org.ovirt.engine.core.common.businessentities.StorageDomainStatic;
import org.ovirt.engine.core.common.businessentities.StorageDomain;
+import org.ovirt.engine.core.common.businessentities.StorageDomainStatic;
import org.ovirt.engine.core.compat.Guid;
public class FCPStorageHelper extends StorageHelperBase {
@@ -16,11 +16,6 @@
@Override
public boolean connectStorageToDomainByVdsId(StorageDomain storageDomain,
Guid vdsId) {
- // Synchronize LUN details comprising the storage domain with the DB
- StorageDomainParametersBase parameters = new
StorageDomainParametersBase(storageDomain.getId());
- parameters.setVdsId(vdsId);
-
Backend.getInstance().runInternalAction(VdcActionType.SyncLunsInfoForBlockStorageDomain,
parameters);
-
return true;
}
@@ -34,4 +29,12 @@
removeStorageDomainLuns(storageDomain);
return true;
}
+
+ @Override
+ public boolean syncDomainInfo(StorageDomain storageDomain, Guid vdsId) {
+ // Synchronize LUN details comprising the storage domain with the DB
+ StorageDomainParametersBase parameters = new
StorageDomainParametersBase(storageDomain.getId());
+ parameters.setVdsId(vdsId);
+ return
Backend.getInstance().runInternalAction(VdcActionType.SyncLunsInfoForBlockStorageDomain,
parameters).getSucceeded();
+ }
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java
index fa435b9..9280464 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java
@@ -65,13 +65,6 @@
isSuccess = returnValue.getSucceeded();
if (isSuccess && VDSCommandType.forValue(type) ==
VDSCommandType.ConnectStorageServer) {
isSuccess = isConnectSucceeded((Map<String, String>)
returnValue.getReturnValue(), list);
-
- if (isSuccess && storageDomain != null) {
- // Synchronize LUN details comprising the storage domain
with the DB
- StorageDomainParametersBase parameters = new
StorageDomainParametersBase(storageDomain.getId());
- parameters.setVdsId(vdsId);
-
Backend.getInstance().runInternalAction(VdcActionType.SyncLunsInfoForBlockStorageDomain,
parameters);
- }
}
}
return isSuccess;
@@ -259,4 +252,12 @@
StorageDomainStatic storageDomain) {
return
DbFacade.getInstance().getStorageServerConnectionDao().getAllForVolumeGroup(storageDomain.getStorage());
}
+
+ @Override
+ public boolean syncDomainInfo(StorageDomain storageDomain, Guid vdsId) {
+ // Synchronize LUN details comprising the storage domain with the DB
+ StorageDomainParametersBase parameters = new
StorageDomainParametersBase(storageDomain.getId());
+ parameters.setVdsId(vdsId);
+ return
Backend.getInstance().runInternalAction(VdcActionType.SyncLunsInfoForBlockStorageDomain,
parameters).getSucceeded();
+ }
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/IStorageHelper.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/IStorageHelper.java
index c31ee27..b646972 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/IStorageHelper.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/IStorageHelper.java
@@ -28,4 +28,6 @@
boolean isConnectSucceeded(Map<String, String> returnValue,
List<StorageServerConnections> connections);
+
+ boolean syncDomainInfo(StorageDomain storageDomain, Guid vdsId);
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHelperBase.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHelperBase.java
index 99ed8bf..bfcfb23 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHelperBase.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHelperBase.java
@@ -194,4 +194,9 @@
}
return "";
}
+
+ @Override
+ public boolean syncDomainInfo(StorageDomain storageDomain, Guid vdsId) {
+ return true;
+ }
}
--
To view, visit http://gerrit.ovirt.org/31274
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I725964aa51ac38bf16d83cc498c5298720031632
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: ovirt-engine-3.5
Gerrit-Owner: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
