Federico Simoncelli has posted comments on this change. Change subject: core: prevent maintenance when domain is still in use ......................................................................
Patch Set 12: (3 comments) http://gerrit.ovirt.org/#/c/22045/12/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 263: if (getParameters().isInactive()) { Line 264: map.setStatus(StorageDomainStatus.InActive); Line 265: } else if (_isLastMaster) { Line 266: map.setStatus(StorageDomainStatus.Maintenance); Line 267: } > I think that "else" with log message is mandatory here Done Line 268: getStoragePoolIsoMapDAO().updateStatus(map.getId(), map.getStatus()); Line 269: if (!Guid.Empty.equals(_newMasterStorageDomainId)) { Line 270: StoragePoolIsoMap mapOfNewMaster = getNewMaster(false).getStoragePoolIsoMapData(); Line 271: mapOfNewMaster.setStatus(StorageDomainStatus.Active); http://gerrit.ovirt.org/#/c/22045/12/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java: Line 209: for (VDS vds : DbFacade.getInstance().getVdsDao().getAllForStoragePool(storagePoolId)) { Line 210: if (vds.getStatus() == VDSStatus.Up Line 211: || vds.getStatus() == VDSStatus.NonResponsive Line 212: || vds.getStatus() == VDSStatus.PreparingForMaintenance Line 213: || vds.getStatus() == VDSStatus.NonOperational) { > +1. I am fine with it but I have a couple of problems: * the name "isConnected" is probably not good enough if used through the entire code base (in fact a vds can be up but not connected to the pool, e.g. when the pool is not initialized yet) * in reality we check three things here: if the vds is up (Up), if it's in the process of going to maintenance (PreparingForMaintenance) or if it's in a state that doesn't give us any information about the storage domains (NonResponsive/NonOperational) So we should come up with a really good name to group these all together or we can just try unify NonResponsive/NonOperational. Line 214: vdsNotInMaintenance.add(vds.getId()); Line 215: } Line 216: } Line 217: http://gerrit.ovirt.org/#/c/22045/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java: Line 1158: boolean isEditAvailable; Line 1159: boolean isActive = storageDomain.getStorageDomainSharedStatus() == StorageDomainSharedStatus.Active Line 1160: || storageDomain.getStorageDomainSharedStatus() == StorageDomainSharedStatus.Mixed; Line 1161: boolean isInMaintenance = (storageDomain.getStatus() == StorageDomainStatus.Maintenance Line 1162: || storageDomain.getStatus() == StorageDomainStatus.PreparingForMaintenance); > Another, method in Enum? I would be fine to group these but officially PreparingForMaintenance is already grouped with Locked (isLocked). Line 1163: boolean isUnattached = (storageDomain.getStorageDomainSharedStatus() == StorageDomainSharedStatus.Unattached); Line 1164: boolean isDataDomain = storageDomain.getStorageDomainType().isDataDomain(); Line 1165: boolean isBlockStorage = storageDomain.getStorageType().isBlockDomain(); Line 1166: -- 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: 12 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: Juan Hernandez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[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
