Liron Aravot has posted comments on this change. Change subject: core: Add vdc query for attached Storage Domain ......................................................................
Patch Set 1: Code-Review-1 (13 comments) http://gerrit.ovirt.org/#/c/36447/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsWithAttachedStoragePoolGuidQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsWithAttachedStoragePoolGuidQuery.java: Line 29: private Guid vdsId; Line 30: protected List<StorageDomainStatic> storageDomainsWithAttachedStoragePoolId = new ArrayList<>(); Line 31: Line 32: public Guid setVdsForConnectStorage() { Line 33: vdsId = getParameters().getVdsId(); what if the host from the parameters is not UP? Line 34: if (vdsId == null) { Line 35: // Get a Host which is at UP state to connect to the Storage Domain. Line 36: List<VDS> hosts = Line 37: getDbFacade().getVdsDao().getAllForStoragePoolAndStatus(getParameters().getId(), VDSStatus.Up); Line 33: vdsId = getParameters().getVdsId(); Line 34: if (vdsId == null) { Line 35: // Get a Host which is at UP state to connect to the Storage Domain. Line 36: List<VDS> hosts = Line 37: getDbFacade().getVdsDao().getAllForStoragePoolAndStatus(getParameters().getId(), VDSStatus.Up); Use RandomUtils- example of usage: RandomUtils.instance().pickRandom(ActionGroup.values()); Line 38: if (!hosts.isEmpty()) { Line 39: vdsId = hosts.get(new Random().nextInt(hosts.size())).getId(); Line 40: log.info("vds id '{}' was chosen to fetch the Storage domain info", vdsId); Line 41: } else { Line 39: vdsId = hosts.get(new Random().nextInt(hosts.size())).getId(); Line 40: log.info("vds id '{}' was chosen to fetch the Storage domain info", vdsId); Line 41: } else { Line 42: log.warn("There is no available vds in UP state to fetch the Storage domain info from VDSM"); Line 43: return null; this return is unneeded, it'll return as null on the next line Line 44: } Line 45: } Line 46: return vdsId; Line 47: } Line 48: Line 49: @Override Line 50: protected void executeQueryCommand() { Line 51: if (setVdsForConnectStorage() == null) { Line 52: return; > set the return value to an empty in this case Is empty correct? how will we know to distinguish between "no attached domains" to "no host to perform the operation"? Line 53: } Line 54: storageDomainsWithAttachedStoragePoolId = filterAttachedStorageDomains(); Line 55: getQueryReturnValue().setReturnValue(storageDomainsWithAttachedStoragePoolId); Line 56: } Line 54: storageDomainsWithAttachedStoragePoolId = filterAttachedStorageDomains(); Line 55: getQueryReturnValue().setReturnValue(storageDomainsWithAttachedStoragePoolId); Line 56: } Line 57: Line 58: protected List<StorageDomainStatic> filterAttachedStorageDomains() { /s/filter/checkFor Line 59: List<StorageDomain> connectedStorageDomainsToVds = new ArrayList<>(); Line 60: for (StorageDomain storageDomain : getParameters().getStorageDomainList()) { Line 61: if (!connectStorageDomain(storageDomain)) { Line 62: logErrorMessage(storageDomain); Line 86: // Storage Pool Line 87: for (StorageDomain storageDomain : storageDomains) { Line 88: try { Line 89: vdsReturnValue = Line 90: runVdsCommand(VDSCommandType.HSMGetStorageDomainInfo, I'd export the execution of HSMGetStorageDomainInfo (and the exception handling) to a different method to shorten the code here. Line 91: new HSMGetStorageDomainInfoVDSCommandParameters(getVdsId(), storageDomain.getId())); Line 92: } catch (RuntimeException e) { Line 93: logErrorMessage(storageDomain); Line 94: continue; Line 87: for (StorageDomain storageDomain : storageDomains) { Line 88: try { Line 89: vdsReturnValue = Line 90: runVdsCommand(VDSCommandType.HSMGetStorageDomainInfo, Line 91: new HSMGetStorageDomainInfoVDSCommandParameters(getVdsId(), storageDomain.getId())); what if we fail to get the domain info and it is attached to a pool? are we checking it somewhere later on the flow or just rely on this query to check that? Line 92: } catch (RuntimeException e) { Line 93: logErrorMessage(storageDomain); Line 94: continue; Line 95: } Line 106: return storageDomainsWithAttachedStoragePoolId; Line 107: } Line 108: Line 109: protected VDSBrokerFrontend getVdsBroker() { Line 110: return Backend.getInstance().getResourceManager(); > use getBackend we can remove that method and inline that in runVdsCommand, that's the only usage of this. Line 111: } Line 112: Line 113: protected VDSReturnValue runVdsCommand(VDSCommandType commandType, VDSParametersBase parameters) Line 114: throws VdcBLLException { Line 132: log.error("Could not get Storage Domain info for Storage Domain (name:'{}', id:'{}') with VDS '{}'. ", Line 133: storageDomain.getName(), Line 134: storageDomain.getId(), Line 135: getVdsId()); Line 136: } else { the logging here is general and used for many cases, I'd change it to be different for each case the ease tracing the flow here. Line 137: log.error("Could not get Storage Domain info with VDS '{}'. ", getVdsId()); Line 138: } Line 139: } http://gerrit.ovirt.org/#/c/36447/1/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsWithAttachedStoragePoolGuidQueryTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsWithAttachedStoragePoolGuidQueryTest.java: Line 36: Line 37: private StorageDomain storageDomain; Line 38: Line 39: @Before Line 40: public void initMocking() { split into methods, also, this not performs just mocking so please rename the method to setup Line 41: storageDomain = new StorageDomain(); Line 42: storageDomain.setStorageName("Name of Storage"); Line 43: storageDomain.setStorageType(StorageType.ISCSI); Line 44: Line 43: storageDomain.setStorageType(StorageType.ISCSI); Line 44: Line 45: VDS vds = new VDS(); Line 46: vds.setId(Guid.newGuid()); Line 47: VdsDAO vdsDAOMock = mock(VdsDAO.class); i think that you should move the mock of the dao to the member decleration, right now you mock some here and some there Line 48: List<VDS> listVds = new ArrayList<>(); Line 49: listVds.add(vds); Line 50: when(vdsDAOMock.getAllForStoragePoolAndStatus(any(Guid.class), eq(VDSStatus.Up))).thenReturn(listVds); Line 51: when(getDbFacadeMockInstance().getVdsDao()).thenReturn(vdsDAOMock); Line 54: doReturn(Boolean.TRUE).when(getQuery()).connectStorageDomain(eq(storageDomain)); Line 55: doReturn(Boolean.TRUE).when(getQuery()).disConnectStorageDomain(eq(storageDomain)); Line 56: } Line 57: Line 58: @Test general - many parts here repeat in all the tests, please export them to methods - we can short things here. Line 59: public void testAttachedStorageDomainQuery() { Line 60: mockVdsCommand(); Line 61: Line 62: // Create parameters http://gerrit.ovirt.org/#/c/36447/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/StorageDomainsAndStoragePoolIdQueryParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/StorageDomainsAndStoragePoolIdQueryParameters.java: Line 24: return storageDomainList; Line 25: } Line 26: Line 27: public void setStorageDomainIdList(List<StorageDomain> storageDomainList) { Line 28: this.storageDomainList = storageDomainList; I'm not in favour of having the caller send the domain list, the data here may not be updated "coffee syndrom") - why not send here domain id's and load it in the query to minimize that risk? it's not likely that we'll encounter bug about that - but it's more correct imo. Line 29: } Line 30: Line 31: public StorageServerConnections getStorageServerConnection() { Line 32: return storageServerConnection; -- To view, visit http://gerrit.ovirt.org/36447 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I749cad223d90dcc8d193765ed842d7e062ad566c Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: [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
