Alissa Bonas has posted comments on this change.
Change subject: core: add ability edit nfs path in webadmin
......................................................................
Patch Set 2: (13 inline comments)
@Liron, yes, there will be REST capability too for that feature, however since
here the UI has full functionality, REST can be supplied in a separate patch.
....................................................
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:
Why do we need it here? (perhaps we do, just want to hear your opinion why)
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;
I'll see what I can do , good idea.
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() {
it is a good idea, I will add.
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 25:
Line 26: @Override
Line 27: protected boolean canDoAction() {
Line 28: StorageServerConnections connection =
getParameters().getStorageServerConnection();
Line 29: //Check if the NFS path has a valid format
it is a good idea, I will add.
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 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
If I remember correctly, there was supposed to be a db constraint on that, but
either way rethinking it - I'll add another check in code to be on the safe
side. good thinking...
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())));
What's the motivation?
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());
OK
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)));
What's the motivation?
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);
What is the common behavior? Are we always "broadcasting" additions/changes to
all VDSMs?
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);
What in your opinion it should be related to?
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 don't see how it should be reused here. There is no adding an existing domain
here, it's repointing an existing one.
The idea here is that there is a storage domain that is always syncronized in
the background on the storage server (think of high availability or disaster
recovery). It is synced from original location A to a backup location B. So the
contents and the metadata are identical. At the point when A becomes down for
some reason, user just repoints it to location B. Since they are identical and
synced, I don't see (and correct me if I'm wrong) what to edit on domain level
here.
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);
On the one hand, it is related to storage domain from user's perspective. On
the other - actually it is a completely different entity that is named "storage
server connection". And it is not touching the tables in db related to storage
domain, only the connections table. So I am not sure that locking it by storage
domain id is the right way here.
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),
In fact it should, however it you look at other action related to storage
server connection - "disconnect", it also has the "Create" action group (which
looks like a bug).
I discussed it with Allon in the past, and we agreed to keep it consistent to
prevent permissions problems, and then have some thinking about it and make
another patch taking care of fixing it for all problematic actions.
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
