Liron Ar has posted comments on this change.
Change subject: core: add ability edit NFS path in webadmin
......................................................................
Patch Set 4: (14 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);
we shouldn't lock here the storage domain
Line 34: }
Line 35:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 29: import org.ovirt.engine.core.utils.transaction.TransactionSupport;
Line 30:
Line 31: @NonTransactiveCommandAttribute
Line 32: public class UpdateStorageServerConnectionCommand<T extends
StorageServerConnectionParametersBase> extends
StorageServerConnectionCommandBase<T> {
Line 33: StorageServerConnections oldConnection = null;
i'd move this so it won't be a member, we use it only in CDA.
Line 34:
Line 35: public UpdateStorageServerConnectionCommand(T parameters) {
Line 36: super(parameters);
Line 37: }
Line 44: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE);
Line 45: }
Line 46:
Line 47: // Check if the NFS path has a valid format
Line 48: if (newConnectionDetails.getstorage_type() == StorageType.NFS
the newConnectionDetails.getstorage_type() == StorageType.NFS check is uneeded,
if it's not NFS the first check would fail
Line 49: && !new
NfsMountPointConstraint().isValid(newConnectionDetails.getconnection(), null)) {
Line 50: return
failCanDoAction(VdcBllMessages.VALIDATION_STORAGE_CONNECTION_INVALID);
Line 51: }
Line 52:
Line 60:
Line 61: // Check that there is no other connection with the new
suggested path
Line 62: List<StorageServerConnections> connections =
Line 63:
getStorageConnDao().getAllForStorage(newConnectionDetails.getconnection());
Line 64: if (connections != null) {
the list will never be null, the dao will return empty list
Line 65: if (connections.size() > 1) {
Line 66: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 67: }
Line 68: else if (connections.size() == 1 &&
!connections.get(0).getid().equals(oldConnection.getid())) {
Line 61: // Check that there is no other connection with the new
suggested path
Line 62: List<StorageServerConnections> connections =
Line 63:
getStorageConnDao().getAllForStorage(newConnectionDetails.getconnection());
Line 64: if (connections != null) {
Line 65: if (connections.size() > 1) {
can we reach that situation?
Line 66: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 67: }
Line 68: else if (connections.size() == 1 &&
!connections.get(0).getid().equals(oldConnection.getid())) {
Line 69: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
Line 65: if (connections.size() > 1) {
Line 66: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 67: }
Line 68: else if (connections.size() == 1 &&
!connections.get(0).getid().equals(oldConnection.getid())) {
Line 69: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
the message seems a bit wrong to me... it should say that the storage
connection already used by this domain, no?
Line 70: }
Line 71:
Line 72: }
Line 73:
Line 71:
Line 72: }
Line 73:
Line 74: List<StorageDomain> domains =
getStorageDomainsByConnId(newConnectionDetails.getid());
Line 75: if (domains != null) {
the list will never be null, the dao will return empty list
Line 76: if (domains.size() != 1) {
Line 77: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 78: }
Line 79: setStorageDomain(domains.get(0));
Line 72: }
Line 73:
Line 74: List<StorageDomain> domains =
getStorageDomainsByConnId(newConnectionDetails.getid());
Line 75: if (domains != null) {
Line 76: if (domains.size() != 1) {
can we reach that situation?
Line 77: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 78: }
Line 79: setStorageDomain(domains.get(0));
Line 80: // Check that the storage domain is in proper state to be
edited
Line 73:
Line 74: List<StorageDomain> domains =
getStorageDomainsByConnId(newConnectionDetails.getid());
Line 75: if (domains != null) {
Line 76: if (domains.size() != 1) {
Line 77: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
is this the correct message? it's also used in line 66
Line 78: }
Line 79: setStorageDomain(domains.get(0));
Line 80: // Check that the storage domain is in proper state to be
edited
Line 81: if (!isNfsPathEditable(getStorageDomain())) {
Line 87: }
Line 88:
Line 89: protected boolean isNfsPathEditable(StorageDomain storageDomain) {
Line 90: boolean isEditable =
Line 91: (storageDomain.getStorageDomainType() ==
StorageDomainType.Data || storageDomain.getStorageDomainType() ==
StorageDomainType.Master)
can't we also edit the patch of export/iso?
Line 92: && storageDomain.getStatus() ==
StorageDomainStatus.Maintenance;
Line 93: return isEditable;
Line 94: }
Line 95:
Line 89: protected boolean isNfsPathEditable(StorageDomain storageDomain) {
Line 90: boolean isEditable =
Line 91: (storageDomain.getStorageDomainType() ==
StorageDomainType.Data || storageDomain.getStorageDomainType() ==
StorageDomainType.Master)
Line 92: && storageDomain.getStatus() ==
StorageDomainStatus.Maintenance;
Line 93: return isEditable;
perhaps just return , we don't need the boolean..
Line 94: }
Line 95:
Line 96: @Override
Line 97: protected void executeCommand() {
Line 93: return isEditable;
Line 94: }
Line 95:
Line 96: @Override
Line 97: protected void executeCommand() {
we have few vdsm operations here that might take some time..perhaps we can lock
the domain the db also for better use experience (so that it will appear as
locked in the gui)
Line 98: //connect to the new path
Line 99: boolean hasConnectStorageSucceeded = connectToStorage();
Line 100:
Line 101: //update info such as free space - because we switched to a
different server
Line 111: return null;
Line 112: }
Line 113: });
Line 114:
Line 115: setSucceeded(true);
I'd suggest adding an audit log in case of failure/success
Line 116: }
Line 117: }
Line 118:
Line 119: protected StorageDomainDynamicDAO getStorageDomainDynamicDao() {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
Line 75: oldConnection.setid(id.toString());
Line 76:
oldConnection.setconnection("multipass.my.domain.tlv.company.com:/export/allstorage/data1");
Line 77: oldConnection.setNfsTimeo((short)50);
Line 78: oldConnection.setstorage_type(StorageType.NFS);
Line 79: oldConnection.setNfsVersion(NfsVersion.V4);
perhaps add a method that create the StorageServerConnections
Line 80:
Line 81:
when(storageConnDao.get(newConnection.getid())).thenReturn(oldConnection);
Line 82:
Line 83: }
--
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: 4
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: Itamar Heim <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
