Alissa Bonas has posted comments on this change.
Change subject: core: add ability edit NFS path in webadmin
......................................................................
Patch Set 6: (16 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
Line 29: }
Line 30:
Line 31: @Override
Line 32: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 33: return
Collections.singletonMap(getStorageDomain().getId().toString(),
LockMessagesMatchUtil.STORAGE);
the idea to lock here is to prevent other people to also try delete it, or for
example someone tries updating a storage domain that someone else is deleting,
so I think the lock is necessary.
I can lock the connection instead of storage domain.
Line 34: }
Line 35:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 38: @Override
Line 39: protected boolean canDoAction() {
Line 40: StorageServerConnections newConnectionDetails =
getParameters().getStorageServerConnection();
Line 41:
Line 42: if (newConnectionDetails.getstorage_type() != StorageType.NFS)
{
I am going to add something similar based on your suggestion without checking
the storage pool - compare the type of old connection details and new one. If
the type is the same, and since when the domain was added to the pool its type
was checked (I checked the code that does that) - then we're good.
Line 43: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE);
Line 44: }
Line 45:
Line 46: // Check if the NFS path has a valid format
Line 43: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE);
Line 44: }
Line 45:
Line 46: // Check if the NFS path has a valid format
Line 47: if (!new
NfsMountPointConstraint().isValid(newConnectionDetails.getconnection(), null)) {
after discussing with Allon - it's not used right now correctly anywhere. so
will prepare a separate patch that handles it for all relevant places.
Line 48: return
failCanDoAction(VdcBllMessages.VALIDATION_STORAGE_CONNECTION_INVALID);
Line 49: }
Line 50:
Line 51: // Check if connection exists by id - otherwise there's
nothing to update
Line 59:
Line 60: // Check that there is no other connection with the new
suggested path
Line 61: List<StorageServerConnections> connections =
Line 62:
getStorageConnDao().getAllForStorage(newConnectionDetails.getconnection());
Line 63: if (connections != null) {
discussed it, will make changes in next patchset, the code of jdbchandler is
misleading...
Line 64: if (connections.size() > 1) {
Line 65: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 66: }
Line 67: else if (connections.size() == 1 &&
!connections.get(0).getid().equals(oldConnection.getid())) {
Line 61: List<StorageServerConnections> connections =
Line 62:
getStorageConnDao().getAllForStorage(newConnectionDetails.getconnection());
Line 63: if (connections != null) {
Line 64: if (connections.size() > 1) {
Line 65: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
discussed it, will make changes in next patchset
Line 66: }
Line 67: else if (connections.size() == 1 &&
!connections.get(0).getid().equals(oldConnection.getid())) {
Line 68: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
Line 69: }
Line 70:
Line 71: }
Line 72:
Line 73: List<StorageDomain> domains =
getStorageDomainsByConnId(newConnectionDetails.getid());
Line 74: if (domains != null) {
discussed, will change
Line 75: if (domains.size() != 1) {
Line 76: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 77: }
Line 78: setStorageDomain(domains.get(0));
Line 92: return isEditable;
Line 93: }
Line 94:
Line 95: @Override
Line 96: protected void executeCommand() {
yes, still not implemented yet.
Line 97: //connect to the new path
Line 98: boolean hasConnectStorageSucceeded = connectToStorage();
Line 99:
Line 100: //update info such as free space - because we switched to a
different server
Line 97: //connect to the new path
Line 98: boolean hasConnectStorageSucceeded = connectToStorage();
Line 99:
Line 100: //update info such as free space - because we switched to a
different server
Line 101: VDSReturnValue returnValueUpdatedStorageDomain =
updateStatsForDomain();
yes you're right
Line 102:
Line 103: if (hasConnectStorageSucceeded &&
returnValueUpdatedStorageDomain.getSucceeded()) {
Line 104: final StorageDomain updatedStorageDomain =
(StorageDomain) returnValueUpdatedStorageDomain.getReturnValue();
Line 105: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
Line 132: return runVdsCommand(VDSCommandType.GetStorageDomainStats,
Line 133: new
GetStorageDomainStatsVDSCommandParameters(getVds().getId(),
getStorageDomain().getId()));
Line 134: }
Line 135:
Line 136:
Done , in next patchset
Line 137:
Line 138: protected StorageServerConnectionDAO getStorageConnDao() {
Line 139: return getDbFacade().getStorageServerConnectionDao();
Line 140: }
Line 136:
Line 137:
Line 138: protected StorageServerConnectionDAO getStorageConnDao() {
Line 139: return getDbFacade().getStorageServerConnectionDao();
Line 140: }
Done, in next patchset
Line 141:
Line 142: protected ConnectStorageServerVDSCommandParameters
createParametersForVdsm(Guid vdsmId,
Line 143: Guid storagePoolId,
Line 144: StorageType storageType,
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
Line 36:
Line 37: @RunWith(MockitoJUnitRunner.class)
Line 38: public class UpdateStorageServerConnectionCommandTest {
Line 39:
Line 40: private LockManager lockManager = new InMemoryLockManager();
you are right, good catch.
Line 41:
Line 42: @Rule
Line 43: public MockEJBStrategyRule ejbRule = new MockEJBStrategyRule();
Line 44:
Line 69: StorageServerConnections oldConnection =
createConnection(id,"multipass.my.domain.tlv.company.com:/export/allstorage/data1",StorageType.NFS,
NfsVersion.V4,50,0);
Line 70:
when(storageConnDao.get(newConnection.getid())).thenReturn(oldConnection);
Line 71: }
Line 72:
Line 73: private StorageServerConnections createConnection(Guid id, String
connection, StorageType type, NfsVersion version, int timeout, int retrans) {
What for? it's a helper method of a setup method in unitest.
Line 74: StorageServerConnections connectionDetails = new
StorageServerConnections();
Line 75: connectionDetails.setid(id.toString());
Line 76: connectionDetails.setconnection(connection);
Line 77: connectionDetails.setNfsVersion(version);
Line 74: StorageServerConnections connectionDetails = new
StorageServerConnections();
Line 75: connectionDetails.setid(id.toString());
Line 76: connectionDetails.setconnection(connection);
Line 77: connectionDetails.setNfsVersion(version);
Line 78: connectionDetails.setNfsTimeo((short)timeout);
Because then the casting will be in the method signature, otherwise it won't
compile.
Line 79: connectionDetails.setstorage_type(type);
Line 80: connectionDetails.setNfsRetrans((short)retrans);
Line 81: return connectionDetails;
Line 82: }
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageServerConnectionDAOTest.java
Line 166: @Test
Line 167: public void testUpdateNfsServerConnection() {
Line 168: //create a new connection
Line 169: StorageServerConnections newNFSServerConnection = new
StorageServerConnections();
Line 170:
newNFSServerConnection.setid("0cb136e8-e5ed-472b-8914-260bc48c2987");
Why?
This is a local temp id for use by a specific test. I don't see any reason to
move it to a different class.
Line 171: newNFSServerConnection.setstorage_type(StorageType.NFS);
Line 172: newNFSServerConnection.setconnection("host/lib/data");
Line 173: newNFSServerConnection.setNfsVersion(NfsVersion.V4);
Line 174: newNFSServerConnection.setNfsRetrans((short) 0);
....................................................
Commit Message
Line 6:
Line 7: core: add ability edit NFS path in webadmin
Line 8:
Line 9: Add the option to edit NFS path and the overriden
Line 10: additional options of storage domain (retransmissions, timeout,
version)
They are not called mount options in the UI.
It's called "Advanced parameters - Override Default Options".
Line 11: in webadmin UI --> Storage tab.
Line 12:
Line 13: Editing those properties is available only when
Line 14: the storage domain is in maintenance status and is data/master domain,
Line 18: Editing is still available for storage domains in status Active/Mixed,
but
Line 19: only for name and description - like it was before this patch.
Line 20:
Line 21: related to bug
Line 22: https://bugzilla.redhat.com/show_bug.cgi?id=835543
do we want build scripts to catch even if it doesn't entirely fix the bug?
Line 23:
Line 24: Change-Id: Ifaff5344ff191d6bdf53cc706c7bb796167a56b3
--
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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
