Alissa Bonas has uploaded a new change for review.
Change subject: core: add validation to AddStorageServerConnection
......................................................................
core: add validation to AddStorageServerConnection
Work in progress...
Prevent addition of a new connection if another one with same
connection details already exists.
Added more unitests for the AddStorageServerConnection command.
Also added VAR__TYPE__STORAGE__CONNECTION to VdcBllMessages, so the connection
related
commands will return a proper message with correct entity type (instead
of returning var type storage domain). AppErrors files already contain
this var type, so no need to add it there.
Change-Id: If9bcef8a413589654d36db8d878c844550ae6066
Signed-off-by: Alissa Bonas <abo...@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/RemoveStorageServerConnectionCommand.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/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
M
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
6 files changed, 112 insertions(+), 36 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/88/15388/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 e38cfb8..fe0d701 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
@@ -20,7 +20,6 @@
import org.ovirt.engine.core.common.validation.group.CreateEntity;
import org.ovirt.engine.core.compat.Guid;
import org.ovirt.engine.core.dal.VdcBllMessages;
-import org.ovirt.engine.core.dal.dbbroker.DbFacade;
@LockIdNameAttribute
@InternalCommandAttribute
@@ -32,18 +31,22 @@
@Override
protected void executeCommand() {
- StorageServerConnections currConnection = getConnection();
+ StorageServerConnections connection =
getParameters().getStorageServerConnection();
boolean isValidConnection = true;
- Pair<Boolean, Integer> result = connect(getVds().getId());
+ Pair<Boolean, Integer> result = connectToStorage();
isValidConnection = result.getFirst();
- // Add storage Connection to the database.
- if (isValidConnection &&
(StringUtils.isNotEmpty(currConnection.getid())
- ||
getDbFacade().getStorageServerConnectionDao().get(currConnection.getid()) ==
null)) {
- currConnection.setid(Guid.NewGuid().toString());
- getDbFacade().getStorageServerConnectionDao().save(currConnection);
- getReturnValue().setActionReturnValue(getConnection().getid());
- setSucceeded(true);
+ // Add storage connection to the database.
+ if (isValidConnection) {
+ //if there was no id sent by client, or if there was sent id, but
this id already exists in db - allocate a new id.
+ //since canDoAction already checks if there is a same connection
in db (same by content, not by id),
+ // we can assume here that if the id received is already taken -
it can be overriden by a new one
+ if(StringUtils.isEmpty(connection.getid()) ||
getConnectionFromDbById(connection.getid()) !=null) {
+ connection.setid(Guid.NewGuid().toString());
+ }
+ saveConnection(connection);
+ getReturnValue().setActionReturnValue(getConnection().getid());
+ setSucceeded(true);
}
else {
VdcFault fault = new VdcFault();
@@ -53,22 +56,32 @@
}
}
- @Override
- protected StorageServerConnections getConnection() {
- if
(StringUtils.isEmpty(getParameters().getStorageServerConnection().getid())) {
- List<StorageServerConnections> connections;
- if ((connections =
DbFacade.getInstance().getStorageServerConnectionDao().getAllForStorage(
-
getParameters().getStorageServerConnection().getconnection())).size() != 0) {
- getParameters().setStorageServerConnection(connections.get(0));
- }
+ protected StorageServerConnections getConnectionFromDbById(String
connectionId) {
+ return getDbFacade().getStorageServerConnectionDao().get(connectionId);
+ }
+
+ protected Pair<Boolean, Integer> connectToStorage() {
+ Guid vdsId = getVds().getId();
+ Pair<Boolean, Integer> result = connect(vdsId);
+ return result;
+ }
+
+ protected void saveConnection(StorageServerConnections connection) {
+ getDbFacade().getStorageServerConnectionDao().save(connection);
+ }
+
+ protected boolean isConnWithSameDetailsExists() {
+ String connection =
getParameters().getStorageServerConnection().getconnection();
+ if
((getDbFacade().getStorageServerConnectionDao().getAllForStorage(connection)).size()
!= 0) {
+ return true;
}
- return (getParameters()).getStorageServerConnection();
+ return false;
}
@Override
protected boolean canDoAction() {
boolean returnValue = true;
- StorageServerConnections paramConnection =
getParameters().getStorageServerConnection();
+ StorageServerConnections paramConnection = getConnection();
if (paramConnection.getstorage_type() == StorageType.NFS
&& !new
NfsMountPointConstraint().isValid(paramConnection.getconnection(), null)) {
returnValue = false;
@@ -76,6 +89,9 @@
}
if (paramConnection.getstorage_type() == StorageType.POSIXFS &&
(StringUtils.isEmpty(paramConnection.getVfsType()))) {
return
failCanDoAction(VdcBllMessages.VALIDATION_STORAGE_CONNECTION_EMPTY_VFSTYPE);
+ }
+ if(isConnWithSameDetailsExists()) {
+ return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
}
if (getParameters().getVdsId().equals(Guid.Empty)) {
@@ -105,4 +121,10 @@
LockMessagesMatchUtil.makeLockingPair(LockingGroup.STORAGE_CONNECTION,
VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED));
}
+
+ @Override
+ protected void setActionMessageParameters() {
+ addCanDoActionMessage(VdcBllMessages.VAR__ACTION__ADD);
+ addCanDoActionMessage(VdcBllMessages.VAR__TYPE__STORAGE__CONNECTION);
+ }
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
index da052fe..a9ed4e1 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
@@ -154,6 +154,6 @@
@Override
protected void setActionMessageParameters() {
addCanDoActionMessage(VdcBllMessages.VAR__ACTION__REMOVE);
- addCanDoActionMessage(VdcBllMessages.VAR__TYPE__STORAGE__DOMAIN);
+ addCanDoActionMessage(VdcBllMessages.VAR__TYPE__STORAGE__CONNECTION);
}
}
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 7646d96..c780460 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
@@ -267,6 +267,6 @@
@Override
protected void setActionMessageParameters() {
addCanDoActionMessage(VdcBllMessages.VAR__ACTION__UPDATE);
- addCanDoActionMessage(VdcBllMessages.VAR__TYPE__STORAGE__DOMAIN);
+ addCanDoActionMessage(VdcBllMessages.VAR__TYPE__STORAGE__CONNECTION);
}
}
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 0027f3f..24b26de 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,20 +1,23 @@
package org.ovirt.engine.core.bll.storage;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.spy;
+
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.runners.MockitoJUnitRunner;
import org.ovirt.engine.core.bll.CanDoActionTestUtils;
+import org.ovirt.engine.core.bll.CommandAssertUtils;
import
org.ovirt.engine.core.common.action.StorageServerConnectionParametersBase;
import org.ovirt.engine.core.common.businessentities.StorageServerConnections;
import org.ovirt.engine.core.common.businessentities.StorageType;
+import org.ovirt.engine.core.common.utils.Pair;
import org.ovirt.engine.core.compat.Guid;
import org.ovirt.engine.core.dal.VdcBllMessages;
import org.ovirt.engine.core.utils.MockEJBStrategyRule;
-
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.spy;
@RunWith(MockitoJUnitRunner.class)
public class AddStorageServerConnectionCommandTest {
@@ -27,13 +30,16 @@
@Before
public void prepareParams() {
- parameters = new StorageServerConnectionParametersBase();
- parameters.setVdsId(Guid.NewGuid());
- parameters.setStoragePoolId(Guid.NewGuid());
- command = spy(new
AddStorageServerConnectionCommand<StorageServerConnectionParametersBase>(parameters));
+ parameters = new StorageServerConnectionParametersBase();
+ parameters.setVdsId(Guid.NewGuid());
+ parameters.setStoragePoolId(Guid.NewGuid());
+ command = spy(new
AddStorageServerConnectionCommand<StorageServerConnectionParametersBase>(parameters));
}
- private StorageServerConnections createPosixConnection(String connection,
StorageType type, String vfsType, String mountOptions) {
+ private StorageServerConnections createPosixConnection(String connection,
+ StorageType type,
+ String vfsType,
+ String mountOptions) {
Guid id = Guid.NewGuid();
StorageServerConnections connectionDetails =
populateBasicConnectionDetails(id, connection, type);
connectionDetails.setVfsType(vfsType);
@@ -49,22 +55,64 @@
return connectionDetails;
}
- @Test
- public void updatePosixEmptyVFSType() {
- StorageServerConnections newPosixConnection =
createPosixConnection("multipass.my.domain.tlv.company.com:/export/allstorage/data1",
StorageType.POSIXFS, null , "timeo=30");
+ @Test
+ public void addPosixEmptyVFSType() {
+ StorageServerConnections newPosixConnection =
+
createPosixConnection("multipass.my.domain.tlv.company.com:/export/allstorage/data1",
+ StorageType.POSIXFS,
+ null,
+ "timeo=30");
parameters.setStorageServerConnection(newPosixConnection);
CanDoActionTestUtils.runAndAssertCanDoActionFailure(command,
VdcBllMessages.VALIDATION_STORAGE_CONNECTION_EMPTY_VFSTYPE);
}
@Test
- public void updatePosixNonEmptyVFSType() {
- StorageServerConnections newPosixConnection =
createPosixConnection("multipass.my.domain.tlv.company.com:/export/allstorage/data1",
StorageType.POSIXFS, "nfs" , "timeo=30");
+ public void addPosixNonEmptyVFSType() {
+ StorageServerConnections newPosixConnection =
+
createPosixConnection("multipass.my.domain.tlv.company.com:/export/allstorage/data1",
+ StorageType.POSIXFS,
+ "nfs",
+ "timeo=30");
parameters.setStorageServerConnection(newPosixConnection);
parameters.setVdsId(Guid.Empty);
doReturn(true).when(command).InitializeVds();
+ doReturn(false).when(command).isConnWithSameDetailsExists();
CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command);
}
+ @Test
+ public void addExistingConnection() {
+ StorageServerConnections newPosixConnection =
+
createPosixConnection("multipass.my.domain.tlv.company.com:/export/allstorage/data1",
+ StorageType.POSIXFS,
+ "nfs",
+ "timeo=30");
+ parameters.setStorageServerConnection(newPosixConnection);
+ parameters.setVdsId(Guid.Empty);
+ doReturn(true).when(command).InitializeVds();
+ doReturn(true).when(command).isConnWithSameDetailsExists();
+ CanDoActionTestUtils.runAndAssertCanDoActionFailure(command,
+
VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
+ }
+ @Test
+ public void addNewConnection() {
+ StorageServerConnections newPosixConnection =
+
createPosixConnection("multipass.my.domain.tlv.company.com:/export/allstorage/data1",
+ StorageType.POSIXFS,
+ "nfs",
+ "timeo=30");
+ newPosixConnection.setid("");
+ parameters.setStorageServerConnection(newPosixConnection);
+ parameters.setVdsId(Guid.Empty);
+ doReturn(true).when(command).InitializeVds();
+ doReturn(false).when(command).isConnWithSameDetailsExists();
+ Pair<Boolean, Integer> connectResult = new Pair(true,0);
+ doReturn(connectResult).when(command).connectToStorage();
+
doReturn(null).when(command).getConnectionFromDbById(newPosixConnection.getid());
+ doNothing().when(command).saveConnection(newPosixConnection);
+ command.executeCommand();
+ CommandAssertUtils.checkSucceeded(command, true);
+ }
}
diff --git
a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
index d695e14..02c756b 100644
---
a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
+++
b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
@@ -507,6 +507,7 @@
ACTION_TYPE_FAILED_UP_VDSS_IN_CLUSTER,
VAR__TYPE__STORAGE__POOL,
VAR__TYPE__STORAGE__DOMAIN,
+ VAR__TYPE__STORAGE__CONNECTION,
VAR__ACTION__ATTACH,
VAR__ACTION__DETACH,
VAR__ACTION__ACTIVATE,
diff --git
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
index 45a7034..51fbe65 100644
---
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
+++
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
@@ -778,8 +778,13 @@
}
private void cleanConnection(StorageServerConnections connection, Guid
hostId) {
- Frontend.RunAction(VdcActionType.RemoveStorageServerConnection, new
StorageServerConnectionParametersBase(connection, hostId),
+ //if create connection command was the one to fail and didn't create a
connection
+ //then the id of connection will be empty, and there's nothing to
delete.
+ if(connection.getid()!=null &&
!connection.getid().equalsIgnoreCase("")) {
+ Frontend.RunAction(VdcActionType.RemoveStorageServerConnection,
new StorageServerConnectionParametersBase(connection, hostId),
null, this);
+ }
+
}
private void remove()
--
To view, visit http://gerrit.ovirt.org/15388
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If9bcef8a413589654d36db8d878c844550ae6066
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <abo...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
