Maor Lipchuk has posted comments on this change.
Change subject: engine: StorageConnections ops won't assume VDS/SP
......................................................................
Patch Set 4: (7 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
Line 31:
Line 32: @Override
Line 33: protected void executeCommand() {
Line 34: boolean success = true;
Line 35: StorageServerConnections connection = getConnection();
please put the connection decleration in the if success condition (At line 51)
, it does not related to the following process
Line 36:
Line 37: // Attempt to connect only if a host is given.
Line 38: // If not, just save the connection to the database
Line 39: if (!Guid.isNullOrEmpty(getParameters().getVdsId())) {
Line 51:
Line 52: if (success) {
Line 53: connection.setid(Guid.newGuid().toString());
Line 54: saveConnection(connection);
Line 55:
getReturnValue().setActionReturnValue(getConnection().getid());
perhaps, worth also change here to use connection.getId() instead
getConnection() again
Line 56: }
Line 57:
Line 58: setSucceeded(success);
Line 59: }
Line 102: if (isConnWithSameDetailsExists(paramConnection)) {
Line 103: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
Line 104: }
Line 105:
Line 106: if (!Guid.isNullOrEmpty(getParameters().getVdsId())) {
I think you should remove this if condition.
now if the vdsId is empty we will return true in the CDA instead of before
where we could returned false.
getParameters().getVdsId() should set mVdsId in the constructor, so getVds()
called here will be null.
Line 107: if (getVds() == null) {
Line 108: return
failCanDoAction(VdcBllMessages.VDS_INVALID_SERVER_ID);
Line 109: }
Line 110: if (getVds().getStatus() != VDSStatus.Up) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DisconnectStorageServerConnectionCommand.java
Line 30: VDSCommandType.DisconnectStorageServer,
Line 31: new
StorageServerConnectionManagementVDSParameters(getParameters().getVdsId(),
Guid.Empty,
Line 32:
getParameters().getStorageServerConnection().getstorage_type(),
Line 33: new ArrayList<>(Arrays
Line 34: .asList(new
StorageServerConnections[]{getConnection()})))).getSucceeded();
Please use the formatter here.
Line 35: }
Line 36:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 190: }
Line 191:
Line 192: @Override
Line 193: protected void executeCommand() {
Line 194: boolean isDomainUpdatePossible =
!Guid.isNullOrEmpty(getVdsId());
+1
Line 195: boolean isDomainUpdateRequired = isDomainUpdatePossible &&
doDomainsUseConnection(getConnection());
Line 196: List<StorageDomain> updatedDomains = new ArrayList<>();
Line 197: boolean hasConnectStorageSucceeded = false;
Line 198: if (isDomainUpdateRequired) {
Line 216: }
Line 217:
getStorageConnDao().update(getParameters().getStorageServerConnection());
Line 218: if (isDomainUpdateRequired) {
Line 219: for (StorageDomain domain : domains) {
Line 220: for (StoragePoolIsoMap map :
getStoragePoolIsoMap(domain)) {
suggestion: I would consider adding a log here, just to avoid cases which we
will have lost domains which will not move back to maintenance from race
issues....
Line 221: restoreStateAfterUpdate(map);
Line 222: }
Line 223: }
Line 224: if (hasConnectStorageSucceeded) {
Line 257: context.snapshotEntityStatus(map,
map.getstatus());
Line 258: updateStatus(map, status);
Line 259: }
Line 260: }
Line 261: getCompensationContext().stateChanged();
Maybe it worth to add a new query which update in one sql execution all the
domains with the status parameter
Line 262: return null;
Line 263: }
Line 264: });
Line 265: }
--
To view, visit http://gerrit.ovirt.org/17288
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I628bf2a63f64d3acaa3520ad144e1eef744f1204
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
