Hello Daniel Erez,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/21255
to review the following change.
Change subject: core: allow identical path for different local domains
......................................................................
core: allow identical path for different local domains
StorageServerConnectionCommandBase -> isConnWithSameDetailsExists:
Added a specific validation for local domains connections;
allowing identical path for local domains residing
on different data-centers.
Note: as a future work, we should add a unique identification
for a local domains connections (e.g. to differentiate between
local connections retrieved from the rest-api).
Change-Id: I4628d7fa442b959d5582e44386f31518b98852e7
Bug-Url: https://bugzilla.redhat.com/1023739
Signed-off-by: Daniel Erez <[email protected]>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageServerConnectionCommandBase.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
M
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommandTest.java
M
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
5 files changed, 87 insertions(+), 25 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/55/21255/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
index f5a9be0..8ab799f 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
@@ -92,7 +92,8 @@
return false;
}
- if (isConnWithSameDetailsExists(paramConnection)) {
+ Guid storagePoolId = Guid.isNullOrEmpty(getParameters().getVdsId()) ?
null : getVds().getStoragePoolId();
+ if (isConnWithSameDetailsExists(paramConnection, storagePoolId)) {
return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageServerConnectionCommandBase.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageServerConnectionCommandBase.java
index 0e76abd..7f75d88 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageServerConnectionCommandBase.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageServerConnectionCommandBase.java
@@ -3,6 +3,7 @@
import java.util.Collections;
import java.util.List;
+import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang.StringUtils;
import org.ovirt.engine.core.bll.CommandBase;
import org.ovirt.engine.core.bll.MultiLevelAdministrationHandler;
@@ -53,9 +54,28 @@
return
getStorageDomainDao().getAllByConnectionId(Guid.createGuidFromString(connectionId));
}
- protected boolean isConnWithSameDetailsExists(StorageServerConnections
connection) {
+ /**
+ * Returns storage pool ID by a specified file domain connection
+ * (isn't relevant for block domains as a single connection can be used by
multiple block domains).
+ */
+ protected Guid getStoragePoolIdByFileConnectionId(String connectionId) {
+ List<StorageDomain> storageDomains =
getStorageDomainsByConnId(connectionId);
+ if (storageDomains.isEmpty()) {
+ return null;
+ }
+
+ return storageDomains.get(0).getStoragePoolId();
+ }
+
+ protected boolean isConnWithSameDetailsExists(StorageServerConnections
connection, Guid storagePoolId) {
List<StorageServerConnections> connections = null;
- if (connection.getstorage_type().isFileDomain()) {
+ if (connection.getstorage_type() == StorageType.LOCALFS) {
+ List<StorageServerConnections> connectionsForPool = storagePoolId
== null ? Collections.<StorageServerConnections> emptyList() :
+
getStorageConnDao().getAllConnectableStorageSeverConnection(storagePoolId);
+ List<StorageServerConnections> connectionsForPath =
getStorageConnDao().getAllForStorage(connection.getconnection());
+ connections = (List<StorageServerConnections>)
CollectionUtils.intersection(connectionsForPool, connectionsForPath);
+ }
+ else if (connection.getstorage_type().isFileDomain()) {
String connectionField = connection.getconnection();
connections =
getStorageConnDao().getAllForStorage(connectionField);
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
index 698e752..f42ad34 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
@@ -84,7 +84,8 @@
return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_CHANGE_STORAGE_TYPE);
}
- if (isConnWithSameDetailsExists(newConnectionDetails)) {
+ Guid storagePoolId =
getStoragePoolIdByFileConnectionId(oldConnection.getid());
+ if (isConnWithSameDetailsExists(newConnectionDetails, storagePoolId)) {
return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
}
diff --git
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommandTest.java
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommandTest.java
index eca6075..5fbacf1 100644
---
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommandTest.java
+++
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommandTest.java
@@ -1,5 +1,6 @@
package org.ovirt.engine.core.bll.storage;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.doReturn;
@@ -7,6 +8,7 @@
import static org.mockito.Mockito.when;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import org.junit.Before;
@@ -23,6 +25,7 @@
import org.ovirt.engine.core.common.errors.VdcBllMessages;
import org.ovirt.engine.core.common.utils.Pair;
import org.ovirt.engine.core.compat.Guid;
+import org.ovirt.engine.core.dao.StorageDomainDAO;
import org.ovirt.engine.core.dao.StorageServerConnectionDAO;
import org.ovirt.engine.core.utils.MockEJBStrategyRule;
@@ -38,12 +41,16 @@
@Mock
StorageServerConnectionDAO storageConnDao;
+ @Mock
+ StorageDomainDAO storageDomainDao;
+
@Before
public void prepareParams() {
parameters = new StorageServerConnectionParametersBase();
parameters.setVdsId(Guid.newGuid());
command = spy(new
AddStorageServerConnectionCommand<StorageServerConnectionParametersBase>(parameters));
doReturn(storageConnDao).when(command).getStorageConnDao();
+ doReturn(storageDomainDao).when(command).getStorageDomainDao();
}
private StorageServerConnections createPosixConnection(String connection,
@@ -96,7 +103,7 @@
"timeo=30");
parameters.setStorageServerConnection(newPosixConnection);
parameters.setVdsId(Guid.Empty);
-
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection,
null);
CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command);
}
@@ -120,7 +127,7 @@
"mypassword123");
parameters.setStorageServerConnection(newISCSIConnection);
parameters.setVdsId(Guid.Empty);
-
doReturn(false).when(command).isConnWithSameDetailsExists(newISCSIConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newISCSIConnection,
null);
CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command);
}
@@ -142,7 +149,7 @@
"timeo=30");
parameters.setStorageServerConnection(newPosixConnection);
parameters.setVdsId(Guid.Empty);
-
doReturn(true).when(command).isConnWithSameDetailsExists(newPosixConnection);
+
doReturn(true).when(command).isConnWithSameDetailsExists(newPosixConnection,
null);
CanDoActionTestUtils.runAndAssertCanDoActionFailure(command,
VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
}
@@ -156,7 +163,7 @@
"timeo=30");
newPosixConnection.setid("");
parameters.setStorageServerConnection(newPosixConnection);
-
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection,
null);
Pair<Boolean, Integer> connectResult = new Pair(true, 0);
doReturn(connectResult).when(command).connectHostToStorage();
doReturn(null).when(command).getConnectionFromDbById(newPosixConnection.getid());
@@ -176,7 +183,7 @@
newPosixConnection.setid("");
parameters.setStorageServerConnection(newPosixConnection);
parameters.setVdsId(Guid.Empty);
-
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection,
null);
doReturn(null).when(command).getConnectionFromDbById(newPosixConnection.getid());
doNothing().when(command).saveConnection(newPosixConnection);
@@ -194,7 +201,7 @@
newPosixConnection.setid("");
parameters.setStorageServerConnection(newPosixConnection);
parameters.setVdsId(null);
-
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection,
null);
doReturn(null).when(command).getConnectionFromDbById(newPosixConnection.getid());
doNothing().when(command).saveConnection(newPosixConnection);
@@ -212,7 +219,7 @@
newPosixConnection.setid(Guid.newGuid().toString());
parameters.setStorageServerConnection(newPosixConnection);
parameters.setVdsId(Guid.Empty);
-
doReturn(true).when(command).isConnWithSameDetailsExists(newPosixConnection);
+
doReturn(true).when(command).isConnWithSameDetailsExists(newPosixConnection,
null);
CanDoActionTestUtils.runAndAssertCanDoActionFailure(command,
VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_NOT_EMPTY);
}
@@ -242,7 +249,40 @@
connections.add(existingConn);
when(storageConnDao.getAllForConnection(newISCSIConnection)).thenReturn(connections);
- boolean isExists =
command.isConnWithSameDetailsExists(newISCSIConnection);
+ boolean isExists =
command.isConnWithSameDetailsExists(newISCSIConnection, null);
assertTrue(isExists);
}
+
+ @Test
+ public void isLocalDomainConnWithSamePathAndPoolExist() {
+ StorageServerConnections newLocalConnection =
populateBasicConnectionDetails("/localSD", StorageType.LOCALFS);
+ StorageServerConnections existingConn =
populateBasicConnectionDetails("/localSD", StorageType.LOCALFS);
+ existingConn.setid(Guid.newGuid().toString());
+
+ Guid storagePoolId = Guid.newGuid();
+ List<StorageServerConnections> connections =
Collections.singletonList(existingConn);
+
+
when(storageConnDao.getAllConnectableStorageSeverConnection(storagePoolId)).thenReturn(connections);
+
when(storageConnDao.getAllForStorage(newLocalConnection.getconnection())).thenReturn(connections);
+
+ boolean isExists =
command.isConnWithSameDetailsExists(newLocalConnection, storagePoolId);
+ assertTrue(isExists);
+ }
+
+ @Test
+ public void isLocalDomainConnWithSamePathAndPoolNotExist() {
+ StorageServerConnections newLocalConnection =
populateBasicConnectionDetails("/localSD", StorageType.LOCALFS);
+ StorageServerConnections existingConn =
populateBasicConnectionDetails("/localSD", StorageType.LOCALFS);
+
+ Guid newLocalConnectionStoragePoolId = Guid.newGuid();
+ Guid existingLocalConnectionStoragePoolId = Guid.newGuid();
+
+ List<StorageServerConnections> connections =
Collections.singletonList(existingConn);
+
+
when(storageConnDao.getAllConnectableStorageSeverConnection(existingLocalConnectionStoragePoolId)).thenReturn(connections);
+
when(storageConnDao.getAllForStorage(newLocalConnection.getconnection())).thenReturn(connections);
+
+ boolean isExists =
command.isConnWithSameDetailsExists(newLocalConnection,
newLocalConnectionStoragePoolId);
+ assertFalse(isExists);
+ }
}
diff --git
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
index db4bea9..87cac4e 100644
---
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
+++
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
@@ -270,7 +270,7 @@
connections.add(conn2);
when(storageConnDao.getAllForStorage(newNFSConnection.getconnection())).thenReturn(connections);
when(storageConnDao.get(newNFSConnection.getid())).thenReturn(oldNFSConnection);
-
doReturn(true).when(command).isConnWithSameDetailsExists(newNFSConnection);
+
doReturn(true).when(command).isConnWithSameDetailsExists(newNFSConnection,
null);
CanDoActionTestUtils.runAndAssertCanDoActionFailure(command,
VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
}
@@ -297,7 +297,7 @@
domains.add(domain2);
when(storageConnDao.get(newNFSConnection.getid())).thenReturn(oldNFSConnection);
doReturn(domains).when(command).getStorageDomainsByConnId(newNFSConnection.getid());
-
doReturn(false).when(command).isConnWithSameDetailsExists(newNFSConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newNFSConnection,
null);
List<String> messages =
CanDoActionTestUtils.runAndAssertCanDoActionFailure(command,
VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
@@ -321,7 +321,7 @@
parameters.setStorageServerConnection(newNFSConnection);
when(storageConnDao.get(newNFSConnection.getid())).thenReturn(oldNFSConnection);
doReturn(domains).when(command).getStorageDomainsByConnId(newNFSConnection.getid());
-
doReturn(false).when(command).isConnWithSameDetailsExists(newNFSConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newNFSConnection,
null);
CanDoActionTestUtils.runAndAssertCanDoActionFailure(command,
VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_DOMAINS_STATUS);
}
@@ -510,7 +510,7 @@
parameters.setStorageServerConnection(newNFSConnection);
when(storageConnDao.get(newNFSConnection.getid())).thenReturn(oldNFSConnection);
doReturn(domains).when(command).getStorageDomainsByConnId(newNFSConnection.getid());
-
doReturn(false).when(command).isConnWithSameDetailsExists(newNFSConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newNFSConnection,
null);
CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command);
}
@@ -545,7 +545,7 @@
domain1.setStatus(StorageDomainStatus.Maintenance);
domains.add(domain1);
when(storageConnDao.get(newNFSConnection.getid())).thenReturn(oldNFSConnection);
-
doReturn(false).when(command).isConnWithSameDetailsExists(newNFSConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newNFSConnection,
null);
doReturn(domains).when(command).getStorageDomainsByConnId(newNFSConnection.getid());
CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command);
}
@@ -566,7 +566,7 @@
parameters.setStorageServerConnection(newPosixConnection);
when(storageConnDao.get(newPosixConnection.getid())).thenReturn(oldPosixConnection);
doReturn(domains).when(command).getStorageDomainsByConnId(newPosixConnection.getid());
-
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection);
+
doReturn(false).when(command).isConnWithSameDetailsExists(newPosixConnection,
null);
CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command);
}
@@ -705,7 +705,7 @@
connections.add(connection2);
when(storageConnDao.getAllForStorage(newNFSConnection.getconnection())).thenReturn(connections);
- boolean isExists =
command.isConnWithSameDetailsExists(newNFSConnection);
+ boolean isExists =
command.isConnWithSameDetailsExists(newNFSConnection, null);
assertTrue(isExists);
}
@@ -723,8 +723,8 @@
connections.add(connection1);
when(storageConnDao.getAllForStorage(newNFSConnection.getconnection())).thenReturn(connections);
- boolean isExists =
command.isConnWithSameDetailsExists(newNFSConnection);
- assertFalse(isExists);
+ boolean isExists =
command.isConnWithSameDetailsExists(newNFSConnection, null);
+ assertFalse(isExists);
}
@Test
@@ -739,7 +739,7 @@
List<StorageServerConnections> connections = new ArrayList<>();
when(storageConnDao.getAllForStorage(newNFSConnection.getconnection())).thenReturn(connections);
- boolean isExists =
command.isConnWithSameDetailsExists(newNFSConnection);
+ boolean isExists =
command.isConnWithSameDetailsExists(newNFSConnection, null);
assertFalse(isExists);
}
@@ -752,7 +752,7 @@
connections.add(connection1);
when(storageConnDao.getAllForConnection(newISCSIConnection)).thenReturn(connections);
- boolean isExists =
command.isConnWithSameDetailsExists(newISCSIConnection);
+ boolean isExists =
command.isConnWithSameDetailsExists(newISCSIConnection, null);
assertTrue(isExists);
}
@@ -764,7 +764,7 @@
connections.add(newISCSIConnection);
when(storageConnDao.getAllForConnection(newISCSIConnection)).thenReturn(connections);
- boolean isExists =
command.isConnWithSameDetailsExists(newISCSIConnection);
- assertFalse(isExists);
+ boolean isExists =
command.isConnWithSameDetailsExists(newISCSIConnection, null);
+ assertFalse(isExists);
}
}
--
To view, visit http://gerrit.ovirt.org/21255
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4628d7fa442b959d5582e44386f31518b98852e7
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: ovirt-engine-3.3.1
Gerrit-Owner: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
