Daniel Erez has uploaded a new change for review. 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 <de...@redhat.com> --- 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/96/21096/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 b620233..ca4b418 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 2fc00dd..9d932ab 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/21096 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 Gerrit-Owner: Daniel Erez <de...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
