Liron Aravot has posted comments on this change.
Change subject: core: add ability edit nfs path in webadmin
......................................................................
Patch Set 2: (12 inline comments)
nice :) , please see in line comments..do you plan to make this usable through
REST as well?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 14: import java.util.ArrayList;
Line 15: import java.util.Arrays;
Line 16: import java.util.Collections;
Line 17: import java.util.Map;
Line 18:
non transactive annotation is missing..
Line 19: public class UpdateStorageServerConnectionCommand<T extends
StorageServerConnectionParametersBase> extends
StorageServerConnectionCommandBase<T> {
Line 20: StorageServerConnections oldConnection = null;
Line 21:
Line 22: public UpdateStorageServerConnectionCommand(T parameters) {
Line 16: import java.util.Collections;
Line 17: import java.util.Map;
Line 18:
Line 19: public class UpdateStorageServerConnectionCommand<T extends
StorageServerConnectionParametersBase> extends
StorageServerConnectionCommandBase<T> {
Line 20: StorageServerConnections oldConnection = null;
perhaps you could add a test for that command..
Line 21:
Line 22: public UpdateStorageServerConnectionCommand(T parameters) {
Line 23: super(parameters);
Line 24: }
Line 23: super(parameters);
Line 24: }
Line 25:
Line 26: @Override
Line 27: protected boolean canDoAction() {
shouldn't the domain status be checked? you should have the validations here
regardless of the gui validation.
Line 28: StorageServerConnections connection =
getParameters().getStorageServerConnection();
Line 29: //Check if the NFS path has a valid format
Line 30: if (connection.getstorage_type() == StorageType.NFS
Line 31: && !new
NfsMountPointConstraint().isValid(connection.getconnection(), null)) {
Line 30: if (connection.getstorage_type() == StorageType.NFS
Line 31: && !new
NfsMountPointConstraint().isValid(connection.getconnection(), null)) {
Line 32: return
failCanDoAction(VdcBllMessages.VALIDATION_STORAGE_CONNECTION_INVALID);
Line 33: }
Line 34: //Check if connection exists by id - otherwise there's nothing
to update
what if we already have a storage server connection to that path?
Line 35: String connectionId = connection.getid();
Line 36: oldConnection =
getDbFacade().getStorageServerConnectionDao().get(connectionId);
Line 37: if(oldConnection == null) {
Line 38: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_NOT_EXIST);
Line 44: protected void executeCommand() {
Line 45: ConnectStorageServerVDSCommandParameters
newConnectionParametersForVdsm = new
ConnectStorageServerVDSCommandParameters(getParameters().getVdsId(),
getParameters()
Line 46: .getStoragePoolId(),
getParameters().getStorageServerConnection().getstorage_type(),
Line 47: new
ArrayList<StorageServerConnections>(Arrays
Line 48:
.asList(getParameters().getStorageServerConnection())));
parhaps Collections.singletonList?
Line 49:
Line 50: VDSReturnValue returnValueConnect =
runVdsCommand(VDSCommandType.ConnectStorageServer,newConnectionParametersForVdsm);
Line 51: if (returnValueConnect.getSucceeded()) {
Line 52:
getDbFacade().getStorageServerConnectionDao().update(getParameters().getStorageServerConnection());
Line 48:
.asList(getParameters().getStorageServerConnection())));
Line 49:
Line 50: VDSReturnValue returnValueConnect =
runVdsCommand(VDSCommandType.ConnectStorageServer,newConnectionParametersForVdsm);
Line 51: if (returnValueConnect.getSucceeded()) {
Line 52:
getDbFacade().getStorageServerConnectionDao().update(getParameters().getStorageServerConnection());
perhaps export the connect/disconnect operations to methods for better
readability
Line 53: //Disconnect the old storage server connection via vdsm -
only after update in db to a new connection succeeds
Line 54: ConnectStorageServerVDSCommandParameters
oldConnectionParametersForVdsm = new
ConnectStorageServerVDSCommandParameters(getParameters().getVdsId(),
getParameters()
Line 55: .getStoragePoolId(),
oldConnection.getstorage_type(),
Line 56: new
ArrayList<StorageServerConnections>(Arrays
Line 53: //Disconnect the old storage server connection via vdsm -
only after update in db to a new connection succeeds
Line 54: ConnectStorageServerVDSCommandParameters
oldConnectionParametersForVdsm = new
ConnectStorageServerVDSCommandParameters(getParameters().getVdsId(),
getParameters()
Line 55: .getStoragePoolId(),
oldConnection.getstorage_type(),
Line 56: new
ArrayList<StorageServerConnections>(Arrays
Line 57: .asList(oldConnection)));
perhaps Collections.singletonList?
Line 58:
runVdsCommand(VDSCommandType.DisconnectStorageServer,oldConnectionParametersForVdsm);
Line 59: setSucceeded(true);
Line 60: }
Line 61: }
Line 54: ConnectStorageServerVDSCommandParameters
oldConnectionParametersForVdsm = new
ConnectStorageServerVDSCommandParameters(getParameters().getVdsId(),
getParameters()
Line 55: .getStoragePoolId(),
oldConnection.getstorage_type(),
Line 56: new
ArrayList<StorageServerConnections>(Arrays
Line 57: .asList(oldConnection)));
Line 58:
runVdsCommand(VDSCommandType.DisconnectStorageServer,oldConnectionParametersForVdsm);
shouldn't this be done for all hosts?
Line 59: setSucceeded(true);
Line 60: }
Line 61: }
Line 62:
Line 55: .getStoragePoolId(),
oldConnection.getstorage_type(),
Line 56: new
ArrayList<StorageServerConnections>(Arrays
Line 57: .asList(oldConnection)));
Line 58:
runVdsCommand(VDSCommandType.DisconnectStorageServer,oldConnectionParametersForVdsm);
Line 59: setSucceeded(true);
if disconnect failed - the operation is already done in the db, so i don't
think that setSucceded should be related to the outcome of the disconnect
command.
Line 60: }
Line 61: }
Line 62:
Line 63: @Override
Line 56: new
ArrayList<StorageServerConnections>(Arrays
Line 57: .asList(oldConnection)));
Line 58:
runVdsCommand(VDSCommandType.DisconnectStorageServer,oldConnectionParametersForVdsm);
Line 59: setSucceeded(true);
Line 60: }
i guess that some functionality should be added here regards the update of the
domain available space and such..take a look at
AddExistingNFSStorageDomainCommand...and possibly you could make some use of
that class (basically that's we are doing here, adding an "existing domain'
whose entities already exist in the db.
Line 61: }
Line 62:
Line 63: @Override
Line 64: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 61: }
Line 62:
Line 63: @Override
Line 64: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 65: return
Collections.singletonMap(getParameters().getStorageServerConnection().getid(),
LockMessagesMatchUtil.STORAGE);
in other domain related flows, the used key for the lock is -
storageDomain.getId()
Line 66: }
Line 67:
Line 68: @Override
Line 69: protected void setActionMessageParameters() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 214: UpdateStoragePool(958,
ActionGroup.EDIT_STORAGE_POOL_CONFIGURATION, QuotaDependency.NONE),
Line 215: FenceVdsManualy(959, ActionGroup.MANIPUTLATE_HOST, false,
QuotaDependency.NONE),
Line 216: AddExistingNFSStorageDomain(960,
ActionGroup.CREATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Line 217: AddStorageServerConnection(1000,
ActionGroup.CREATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Line 218:
UpdateStorageServerConnection(1001,ActionGroup.CREATE_STORAGE_DOMAIN,QuotaDependency.NONE),
shouldn't the action group be ActionGroup.EDIT_STORAGE_DOMAIN_CONFIGURATION?
Line 219: DisconnectStorageServerConnection(1002,
ActionGroup.CREATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Line 220: ConnectHostToStoragePoolServers(1004, QuotaDependency.NONE),
Line 221: DisconnectHostFromStoragePoolServers(1005, QuotaDependency.NONE),
Line 222: ConnectStorageToVds(1006, ActionGroup.CREATE_STORAGE_DOMAIN,
QuotaDependency.NONE),
--
To view, visit http://gerrit.ovirt.org/12372
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaff5344ff191d6bdf53cc706c7bb796167a56b3
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
