Alissa Bonas has posted comments on this change.
Change subject: core,restapi: attach detach storage connections
......................................................................
Patch Set 4:
(11 comments)
of course, unitests need to be added to backend/dao/rest...
just a reminder, I know it is still wip. :)
Also, perhaps I missed it but which code prevents attach/detach for file
domains?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DetachStorageConnectionFromStorageDomainCommand.java
Line 68: }
Line 69:
Line 70: @Override
Line 71: protected void setActionMessageParameters() {
Line 72: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__DETACH);
Here you are using VAR_ACTION_DETACH, but in the attach command you are using
VAR_ACTION_ATTACH_TO.
I checked the file, and for some reason there are 2 attach (VAR_ACTION_ATTACH,
VAR__ACTION__ATTACH_ACTION_TO) and 2 detach (VAR_ACTION_DETACH,
VAR__ACTION__DETACH_ACTION_TO) actions with same value ($action attach, $action
detach) in VdcBllMessages - any idea why?
I know this duplicity is not related to this patch, but at least I suggest
using symmetrical VAR_ACTION_XYZ here and in the attach command.
Line 73:
addCanDoActionMessage(VdcBllMessages.VAR__TYPE__STORAGE__CONNECTION);
Line 74: }
Line 75:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageConnectionValidator.java
Line 9: import
org.ovirt.engine.core.common.businessentities.StorageServerConnections;
Line 10: import org.ovirt.engine.core.common.businessentities.StorageType;
Line 11: import org.ovirt.engine.core.common.errors.VdcBllMessages;
Line 12:
Line 13: public class StorageConnectionValidator {
I don't think that adding this validator as part of this specific patch is a
good idea.
I would do it as a separate patch and move all relevant commands of storage
connection management to use it.
Otherwise - it's not really required by the current patch functionality, and
there's code duplication among the rest of the commands.
What do you think?
Line 14: protected static final String STORAGE_DOMAIN_NAME_REPLACEMENT =
"$domainNames %1$s";
Line 15:
Line 16: private StorageServerConnections connection;
Line 17:
Line 26:
Line 27: return ValidationResult.VALID;
Line 28: }
Line 29:
Line 30: public ValidationResult isScsiConnectionAndDomain(StorageDomain
storageDomain) {
iscsi should be all in upperCase (except for the first i).
And the name of the method is misleading. I suggest naming it
isConnAndDomainOfSameType.
I would pass here as parameters storage domain, connection and the required
storage type and check if both connection and domain are of the same type.
This way it can be reused by all storage types.
Line 31: StorageType connectionStorageType =
connection.getstorage_type();
Line 32: StorageType storageDomainType = storageDomain.getStorageType();
Line 33:
Line 34: if (!connectionStorageType.equals(StorageType.ISCSI) ||
!storageDomainType.equals(StorageType.ISCSI)) {
Line 42: if (storageDomain == null) {
Line 43: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST);
Line 44: }
Line 45: if (storageDomain.getStatus() !=
StorageDomainStatus.Maintenance
Line 46: && storageDomain.getStatus() !=
StorageDomainStatus.Unattached) {
this won't work. unattached is the domain shared status, the status will return
unknown.
Line 47: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_DOMAINS_MAINTENANCE,
Line 48: String.format(STORAGE_DOMAIN_NAME_REPLACEMENT,
storageDomain.getStorageName()));
Line 49: }
Line 50:
Line 50:
Line 51: return ValidationResult.VALID;
Line 52: }
Line 53:
Line 54: public ValidationResult
isConnectionForDomainAlreadyExists(LUN_storage_server_connection_map
connectionMapRecord) {
this is valid only for block domains. what about file domains?
Line 55: if (connectionMapRecord != null) {
Line 56: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_ALREADY_EXISTS);
Line 57: }
Line 58:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 227: MoveDisks(1012, false, QuotaDependency.NONE),
Line 228: ExtendImageSize(1013, false, QuotaDependency.STORAGE),
Line 229: ImportRepoImage(1014, ActionGroup.CREATE_DISK,
QuotaDependency.STORAGE),
Line 230: ExportRepoImage(1015, QuotaDependency.NONE),
Line 231: AttachStorageConnectionToStorageDomain(1016,
ActionGroup.MANIPULATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Currently we don't have permissions in other connections management actions.
Is it good to introduce it only here?
Line 232: DetachStorageConnectionFromStorageDomain(1017,
ActionGroup.MANIPULATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Line 233:
Line 234: // Event Notification
Line 235: AddEventSubscription(1100, false, QuotaDependency.NONE),
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java
Line 455:
ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST(ErrorType.BAD_PARAMETERS),
Line 456:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_NOT_EXIST(ErrorType.BAD_PARAMETERS),
Line 457:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_NOT_EXIST(ErrorType.BAD_PARAMETERS),
Line 458:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS(ErrorType.BAD_PARAMETERS),
Line 459:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_ALREADY_EXISTS(ErrorType.BAD_PARAMETERS),
maybe the type ErrorType.CONFLICT is more appropriate?
Line 460:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE_TYPE(ErrorType.NOT_SUPPORTED),
Line 461:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_DOMAINS_MAINTENANCE(ErrorType.NOT_SUPPORTED),
Line 462:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_RUNNING_VMS_AND_DOMAINS_MAINTENANCE(ErrorType.NOT_SUPPORTED),
Line 463:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_RUNNING_VMS(ErrorType.NOT_SUPPORTED),
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 1188: parameterType: null
Line 1189: signatures: []
Line 1190: urlparams: {}
Line 1191: headers: {}
Line 1192: - name:
/api/storagedomains/{storagedomain:id}/storageconnections|rel=add
as I mentioned in other file, the add/delete are imho misleading - we are not
creating nor deleting any entity, just relation between 2 entities. User might
think we add/delete a new storage connection while we are just reusing an
existing one.
Line 1193: request:
Line 1194: body:
Line 1195: parameterType: StorageConnection
Line 1196: signatures:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainServerConnectionsResource.java
Line 53: }
Line 54:
Line 55: @Override
Line 56: public Response add(StorageConnection storageConnection) {
Line 57: validateParameters(storageConnection, "id");
as I mentioned, I don't really like calling it an "add" - it's not adding any
new entity to the system. But let's hear other opinions as well...
Line 58:
Line 59: return
performAction(VdcActionType.AttachStorageConnectionToStorageDomain,
Line 60: new
AttachDetachStorageConnectionParameters(storageDomainId,
storageConnection.getId()));
Line 61: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/utils/FeaturesHelper.java
Line 330:
Line 331: private void
addFeatureAttachDetachStorageServerConnectionsForDomain(Features features) {
Line 332: Feature feature = new Feature();
Line 333: feature.setName("Attach/Detach storage server connections
(to/from a storage domain)");
Line 334: feature.setDescription("Ability to attach/detach storage
server connections to/from a specific storage domain (common use case: disaster
recovery).");
I would mention that is relevant only for block domains
Line 335: features.getFeature().add(feature);
Line 336: }
....................................................
File
frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 372: ACTION_TYPE_FAILED_NAME_ALREADY_USED=Cannot ${action} ${type}. The
${type} name is already in use, please choose a unique name and try again.
Line 373: VALIDATION_VDS_CONSOLEADDRESSS_HOSTNAME_OR_IP=Console address must be
a FQDN or a valid IP address
Line 374: ACTION_TYPE_FAILED_URL_INVALID=Cannot ${action} ${type}. The URL is
not valid, please enter a valid URL and try again.
Line 375: ACTION_TYPE_FAILED_STORAGE_CONNECTION_NOT_EXIST=Cannot ${action}
${type}. Storage connection doesn't exist.
Line 376: ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_NOT_EXIST=Cannot
${action} ${type}. Storage connection is not attached to the specified domain.
just an idea - how about adding $domainName and specifying the domain name?
Line 377: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY=Cannot ${action}
${type}. Storage connection id is empty.
Line 378: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_NOT_EMPTY=Cannot ${action}
${type}. Storage connection id is not empty.
Line 379: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS=Cannot ${action}
${type}. Storage connection already exists.
Line 380:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_ALREADY_EXISTS=Cannot
${action} ${type}. Storage connection is already attached to the specified
domain.
--
To view, visit http://gerrit.ovirt.org/17864
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I319bc9f51c81e2df0c7ed3b3338fcd7b4783fe84
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
