Liron Ar has posted comments on this change. Change subject: core: prevent maintenance when domain is still in use ......................................................................
Patch Set 6: (7 comments) http://gerrit.ovirt.org/#/c/22045/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainCommand.java: Line 164: (new StoragePoolIsoMapId(getParameters().getStorageDomainId(), Line 165: getParameters().getStoragePoolId())); Line 166: map.setStatus(StorageDomainStatus.Unknown); Line 167: changeStorageDomainStatusInTransaction(map, Line 168: getParameters().isInactive() ? StorageDomainStatus.Locked : StorageDomainStatus.MovingToMaintenance); this change may be omitted, you could change it in line 232 to moving to maintenance. Line 169: proceedStorageDomainTreatmentByDomainType(false); Line 170: Line 171: if (_isLastMaster) { Line 172: executeInNewTransaction(new TransactionMethod<Object>() { Line 172: executeInNewTransaction(new TransactionMethod<Object>() { Line 173: @Override Line 174: public Object runInTransaction() { Line 175: getCompensationContext().snapshotEntityStatus(getStoragePool()); Line 176: getStoragePool().setStatus(StoragePoolStatus.Maintenance); if it's the last master the pool shouldn't move to maintenance while the domain is "moving to maintenance" otherwise we might not be able to get out of this situation. Line 177: getStoragePoolDAO().updateStatus(getStoragePool().getId(), getStoragePool().getStatus()); Line 178: getCompensationContext().stateChanged(); Line 179: return null; Line 180: } Line 229: public Object runInTransaction() { Line 230: if (getParameters().isInactive()) { Line 231: map.setStatus(StorageDomainStatus.InActive); Line 232: } Line 233: getStoragePoolIsoMapDAO().updateStatus(map.getId(), map.getStatus()); this update should move to the if clause as well now. Line 234: if (!Guid.Empty.equals(_newMasterStorageDomainId)) { Line 235: StoragePoolIsoMap mapOfNewMaster = getNewMaster(false).getStoragePoolIsoMapData(); Line 236: mapOfNewMaster.setStatus(StorageDomainStatus.Active); Line 237: getStoragePoolIsoMapDAO().updateStatus(mapOfNewMaster.getId(), mapOfNewMaster.getStatus()); http://gerrit.ovirt.org/#/c/22045/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ReconstructMasterDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ReconstructMasterDomainCommand.java: Line 51: canChooseCurrentMasterAsNewMaster = parameters.isCanChooseCurrentMasterAsNewMaster(); Line 52: } Line 53: Line 54: private boolean checkIsDomainLocked(StoragePoolIsoMap domainMap) { Line 55: if (domainMap.getStatus().isLocked()) { i don't think this is correct, we should be still able to reconstruct, just not to domain in "moving to maintenance" status otherwise we might be never able to recover. Line 56: addInvalidSDStatusMessage(domainMap.getStatus()); Line 57: return true; Line 58: } Line 59: http://gerrit.ovirt.org/#/c/22045/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java: Line 163: if (status != null) { Line 164: valid = Arrays.asList(statuses).contains(status); Line 165: } Line 166: if (!valid) { Line 167: if (status.isLocked()) { that'll also block editing the domain (name for example), is that what we want in that case as well? Line 168: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED); Line 169: } Line 170: addStorageDomainStatusIllegalMessage(); Line 171: } http://gerrit.ovirt.org/#/c/22045/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java: Line 279: Line 280: } Line 281: Line 282: // DRAFT COMMENT: in the long run this will replace updateVdsDomainsData Line 283: IrsBrokerCommand.updateVdsDomainsVisibility(tmpVds, domainsList); this line is exact copy of the line below Line 284: Line 285: // Now update the status of domains, this code should not be in Line 286: // synchronized part of code Line 287: IrsBrokerCommand.updateVdsDomainsVisibility(tmpVds, domainsList); http://gerrit.ovirt.org/#/c/22045/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/DomainVisibility.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/DomainVisibility.java: Line 87: public synchronized Status getDomainStatusByVds(Guid domainId, Guid vdsId) { Line 88: Map<Guid, Status> domainVdsStatus = domainsVisibility.get(domainId); Line 89: Line 90: if (domainVdsStatus.containsKey(vdsId)) { Line 91: return domainVdsStatus.get(vdsId); better to just get and check if null Line 92: } Line 93: Line 94: return Status.UNKNOWN; Line 95: } -- To view, visit http://gerrit.ovirt.org/22045 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I55cd5aa6a6dc32f374a4bb21b159d3cb30da65f5 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[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
