Alissa Bonas has posted comments on this change.
Change subject: core: add canDoAction to remove storage connection
......................................................................
Patch Set 4: (14 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
Line 32: protected boolean canDoAction() {
Line 33: String connectionId = getConnection().getid();
Line 34: List<StorageDomain> domains = new ArrayList<>();
Line 35: if(StringUtils.isEmpty(connectionId) ) {
Line 36: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY);
Annotating that will not be good because:
1. minor reason : currently it's only used in this command, so it's not very
interesting to annotate just for this.
2. major reason: the same connection object is used also by
AddStorageServerConnectionCommand, which actually checks the opposite - it
requires the id to be empty, and fails if it's not empty. So the annotation
will be wrong in this case.
Line 37: }
Line 38: StorageServerConnections connection =
getStorageServerConnectionDao().get(connectionId);
Line 39: StorageType storageType = connection.getstorage_type();
Line 40: if(storageType.isFileDomain()) {
Line 34: List<StorageDomain> domains = new ArrayList<>();
Line 35: if(StringUtils.isEmpty(connectionId) ) {
Line 36: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY);
Line 37: }
Line 38: StorageServerConnections connection =
getStorageServerConnectionDao().get(connectionId);
Currently, the backend cannot trust the GUI to fill all needed fields (needed
by the command), thus it's brought from db. In the long term, I agree that it's
better to get just id from the client in the case of deletion , because it's
really all we need from the client in this case. However, I wouldn't do it in
scope of this patch.
Line 39: StorageType storageType = connection.getstorage_type();
Line 40: if(storageType.isFileDomain()) {
Line 41: //go to storage domain static, get all storage domains
where storage field = storage connection id
Line 42: domains = getStorageDomainsByConnId(connectionId);
Line 46: }
Line 47: }
Line 48: else if(storageType.equals(StorageType.ISCSI)) {
Line 49: List<String> domainNames = new ArrayList<>();
Line 50: List<String> diskNames = new ArrayList<>();
why linked?
Line 51: //go to luns to storage connections map table, get it from
there
Line 52: List<LUNs> luns =
getLunDao().getAllForStorageServerConnection(connectionId);
Line 53: if(luns.size() > 0) {
Line 54: String volumeGroupId = null;
Line 65: diskNames.add(lunDiskName);
Line 66: }
Line 67: }
Line 68: String domainNamesForMessage = null;
Line 69: if(domainNames.size() > 0 ) {
Done
Line 70: // Build domain names list to display in the error
Line 71: domainNamesForMessage =
prepareEntityNamesForMessage(domainNames);
Line 72: if(diskNames.size() < 1) {
Line 73: return
prepareFailureMessageForDomains(domainNamesForMessage) ;
Line 68: String domainNamesForMessage = null;
Line 69: if(domainNames.size() > 0 ) {
Line 70: // Build domain names list to display in the error
Line 71: domainNamesForMessage =
prepareEntityNamesForMessage(domainNames);
Line 72: if(diskNames.size() < 1) {
Done
Line 73: return
prepareFailureMessageForDomains(domainNamesForMessage) ;
Line 74: }
Line 75: else {
Line 76: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 71: domainNamesForMessage =
prepareEntityNamesForMessage(domainNames);
Line 72: if(diskNames.size() < 1) {
Line 73: return
prepareFailureMessageForDomains(domainNamesForMessage) ;
Line 74: }
Line 75: else {
sorry, didn't understand your comment - what do you mean by unwrap this?
Line 76: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 77: return
prepareFailureMessageForDomainsAndDisks(domainNamesForMessage,diskNamesForMessage);
Line 78: }
Line 79: }
Line 76: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 77: return
prepareFailureMessageForDomainsAndDisks(domainNamesForMessage,diskNamesForMessage);
Line 78: }
Line 79: }
Line 80: else if(diskNames.size() > 0 ) {
Done
Line 81: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 82: return
prepareFailureMessageForDisks(diskNamesForMessage);
Line 83: }
Line 84:
Line 99: domainNamesForMessage.append(",");
Line 100: }
Line 101: // Remove the last "," after the last domain
Line 102:
domainNamesForMessage.deleteCharAt(domainNamesForMessage.length() - 1);
Line 103: return domainNamesForMessage.toString();
Done
Line 104: }
Line 105:
Line 106: @Override
Line 107: protected void executeCommand() {
Line 132: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 133: return
Collections.singletonMap(getConnection().getconnection(),
Line 134:
LockMessagesMatchUtil.makeLockingPair(LockingGroup.STORAGE_CONNECTION,
Line 135:
VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED));
Line 136: }
Done
Line 137:
Line 138: protected String
createDomainNamesListFromStorageDomains(List<StorageDomain> domains) {
Line 139: // Build domain names list to display in the error
Line 140: StringBuilder domainNames = new StringBuilder();
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommandTest.java
Line 28: import static org.mockito.Mockito.when;
Line 29:
Line 30: @RunWith(MockitoJUnitRunner.class)
Line 31: public class RemoveStorageServerConnectionCommandTest {
Line 32: @ClassRule
reformatted again.
Line 33: public static MockEJBStrategyRule ejbRule = new
MockEJBStrategyRule();
Line 34:
Line 35: private RemoveStorageServerConnectionCommand command = null;
Line 36:
Line 86: Guid id = Guid.NewGuid();
Line 87: StorageServerConnections connectionDetails =
populateBasicConnectionDetails(id, connection, type);
Line 88: connectionDetails.setNfsVersion(version);
Line 89: connectionDetails.setNfsTimeo((short) timeout);
Line 90: connectionDetails.setNfsRetrans((short) retrans);
because then it has to be passed as Short, otherwise putting inline primitive
numbers makes the compiler not match it to the method signature,
Line 91: return connectionDetails;
Line 92: }
Line 93:
Line 94: private StorageServerConnections createIscsiConnection(String
connection,
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
Line 453: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS,
Line 454:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE,
Line 455:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS,
Line 456:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS,
Line 457: ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL__DISKS,
yes, already saw it myself, but good catch, thanks
Line 458:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_WRONG_PARAMETERS_FOR_STORAGE_TYPE,
Line 459:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_CHANGE_STORAGE_TYPE,
Line 460: ACTION_TYPE_FAILED_STORAGE_DOMAIN_STATUS_ILLEGAL,
Line 461: ACTION_TYPE_FAILED_STORAGE_DOMAIN_STATUS_ILLEGAL2,
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 343: ACTION_TYPE_FAILED_STORAGE_CONNECTION_NOT_EXIST=Cannot ${action}
${type}. Storage connection doesn't exist.
Line 344: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY=Cannot ${action}
${type}. Storage connection id is empty.
Line 345: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS=Cannot ${action}
${type}. Storage connection already exists.
Line 346:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE=Cannot
${action} ${type}. Storage connection parameters can be edited only for NFS
data domains in maintenance.
Line 347:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS=Cannot
${action} ${type}. Cannot ${action} ${type}. Storage connection parameters are
related to the following storage domains : ${domainNames}.
Done
Line 348:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS=Cannot
${action} ${type}. Storage connection parameters are related to the following
storage domains : ${domainNames} and disks: ${diskNames}.
Line 349:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL__DISKS=Cannot
${action} ${type}. Storage connection parameters are related to the following
disks : ${diskNames}.
Line 350:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_WRONG_PARAMETERS_FOR_STORAGE_TYPE=Cannot
${action} ${type}. Connection parameters are invalid for this storage type.
Line 351:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_CHANGE_STORAGE_TYPE=Cannot
${action} ${type}. Storage type cannot be edited.
Line 344: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY=Cannot ${action}
${type}. Storage connection id is empty.
Line 345: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS=Cannot ${action}
${type}. Storage connection already exists.
Line 346:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE=Cannot
${action} ${type}. Storage connection parameters can be edited only for NFS
data domains in maintenance.
Line 347:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS=Cannot
${action} ${type}. Cannot ${action} ${type}. Storage connection parameters are
related to the following storage domains : ${domainNames}.
Line 348:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS=Cannot
${action} ${type}. Storage connection parameters are related to the following
storage domains : ${domainNames} and disks: ${diskNames}.
Done
Line 349:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL__DISKS=Cannot
${action} ${type}. Storage connection parameters are related to the following
disks : ${diskNames}.
Line 350:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_WRONG_PARAMETERS_FOR_STORAGE_TYPE=Cannot
${action} ${type}. Connection parameters are invalid for this storage type.
Line 351:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_CHANGE_STORAGE_TYPE=Cannot
${action} ${type}. Storage type cannot be edited.
Line 352: ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST=Cannot ${action} ${type}.
Storage Domain doesn't exist.
--
To view, visit http://gerrit.ovirt.org/15269
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7a633b06195a7b8ca1cd4fc5023c05010a3161d
Gerrit-PatchSet: 4
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: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
