Maor Lipchuk has posted comments on this change. Change subject: core: Add vdc query for attached Storage Domain ......................................................................
Patch Set 1: (23 comments) We didn't continue with the patches before since all the logic here was changed (separate one query to three others) 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 25: public GetStorageDomainsWithAttachedStoragePoolGuidQuery(P parameters) { Line 26: super(parameters); Line 27: } Line 28: Line 29: private Guid vdsId; > move members to top done Line 30: protected List<StorageDomainStatic> storageDomainsWithAttachedStoragePoolId = new ArrayList<>(); Line 31: Line 32: public Guid setVdsForConnectStorage() { Line 33: vdsId = getParameters().getVdsId(); Line 26: super(parameters); Line 27: } Line 28: Line 29: private Guid vdsId; Line 30: protected List<StorageDomainStatic> storageDomainsWithAttachedStoragePoolId = new ArrayList<>(); > why initialization is needed? removed the parameter Line 31: Line 32: public Guid setVdsForConnectStorage() { Line 33: vdsId = getParameters().getVdsId(); Line 34: if (vdsId == null) { Line 28: Line 29: private Guid vdsId; Line 30: protected List<StorageDomainStatic> storageDomainsWithAttachedStoragePoolId = new ArrayList<>(); Line 31: Line 32: public Guid setVdsForConnectStorage() { > it's a bit weird that a setter returns a value... rename to getVdsForConnec done 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 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? It is the same logic we use now for attach, the query will simply fail to execute the VDS command and will return an empty list (and the operation will fail later) 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: This is the same logic done today in the attach, no need to change it 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 removed 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 done, We will fail later, and the logs will indicate it 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 I prefer to leave this that way Line 59: List<StorageDomain> connectedStorageDomainsToVds = new ArrayList<>(); Line 60: for (StorageDomain storageDomain : getParameters().getStorageDomainList()) { Line 61: if (!connectStorageDomain(storageDomain)) { Line 62: logErrorMessage(storageDomain); Line 66: } Line 67: Line 68: storageDomainsWithAttachedStoragePoolId = getAttachedStorageDomains(connectedStorageDomainsToVds); Line 69: for (StorageDomain storageDomain : connectedStorageDomainsToVds) { Line 70: if (!disConnectStorageDomain(storageDomain)) { > /s/disConnect/disconnect done Line 71: log.warn("Could not disconnect Storage Domain {} from VDS '{}'. ", storageDomain.getName(), getVdsId()); Line 72: } Line 73: } Line 74: return storageDomainsWithAttachedStoragePoolId; Line 81: protected List<StorageDomainStatic> getAttachedStorageDomains(List<StorageDomain> storageDomains) { Line 82: VDSReturnValue vdsReturnValue = null; Line 83: List<StorageDomainStatic> storageDomainsWithAttachedStoragePoolId = new ArrayList<>(); Line 84: Line 85: // Go over the list of Storage Domains and try to get the Storage Domain info to check if it attached to another > s/it/it is done Line 86: // Storage Pool Line 87: for (StorageDomain storageDomain : storageDomains) { Line 88: try { Line 89: vdsReturnValue = 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 hand I prefer to leave this that way 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 the command later should use the same vds command so we will fail later, therefore we should not do nothing, reminding you, this is only a query not a command. 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 changing the getBackend, I prefer to leave it that way for test reasons Line 111: } Line 112: Line 113: protected VDSReturnValue runVdsCommand(VDSCommandType commandType, VDSParametersBase parameters) Line 114: throws VdcBLLException { Line 120: .getItem(storageDomain.getStorageType()) Line 121: .connectStorageToDomainByVdsId(storageDomain, getVdsId()); Line 122: } Line 123: Line 124: protected boolean disConnectStorageDomain(StorageDomain storageDomain) { > s/disConnect/disconnect done Line 125: return StorageHelperDirector.getInstance() Line 126: .getItem(storageDomain.getStorageType()) Line 127: .disconnectStorageFromDomainByVdsId(storageDomain, getVdsId()); Line 128: } Line 131: if (storageDomain != null) { 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()); > should probably check geVdsId() nullity as well We could not get here if the vds id is null, that is why I added the return here in line 52 Line 136: } else { Line 137: log.error("Could not get Storage Domain info with VDS '{}'. ", getVdsId()); Line 138: } Line 139: } 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 di I prefer to leave it that way, it is better then nothing 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, renamed method, I prefer not to split it 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, There is no need to do so, this mock is being used before every test while the other mocking is being used per test 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 me I prefer to leave it that way, it makes the tests more flexible and maintainable 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 11: private List<StorageDomain> storageDomainList; Line 12: private StorageServerConnections storageServerConnection; Line 13: private Guid vdsId; Line 14: Line 15: public Guid getVdsId() { > convention - move getters/setters to the bottom done Line 16: return vdsId; Line 17: } Line 18: Line 19: public void setVdsId(Guid vdsId) { 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 The commands in the process should prevent this, It is more convenient to pass Storage Domains GUI wise Line 29: } Line 30: Line 31: public StorageServerConnections getStorageServerConnection() { Line 32: return storageServerConnection; Line 38: Line 39: public StorageDomainsAndStoragePoolIdQueryParameters() { Line 40: } Line 41: Line 42: public StorageDomainsAndStoragePoolIdQueryParameters(List<StorageDomain> storageDomainList, Guid storagePoolId, Guid vdsId) { > consider adding a ctr without vdsId (for attach usage) done Line 43: super(storagePoolId); Line 44: this.storageDomainList = storageDomainList; Line 45: this.vdsId = vdsId; Line 46: } http://gerrit.ovirt.org/#/c/36447/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java: Line 373: GetAttachedClustersByClusterPolicyId, Line 374: GetAffinityGroupById, Line 375: GetAffinityGroupsByClusterId, Line 376: GetAffinityGroupsByVmId, Line 377: GetStorageDomainsWithAttachedStoragePoolGuid, > this is Scheduling section, move it to a more suitable location... done Line 378: Line 379: GetAllDisksPartialDataByVmId(VdcQueryAuthType.User), Line 380: GetVmTemplateCount, Line 381: -- 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: 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
