Nir Soffer has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 5: (13 comments) https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RefreshLunsSizeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RefreshLunsSizeCommand.java: Line 49: if (!(checkStorageDomain() && checkStorageDomainStatus(StorageDomainStatus.Active))) { Line 50: return false; Line 51: } Line 52: Line 53: if ((getStorageDomain().getStorageType() == StorageType.NFS || getStorageDomain().getStorageType() == StorageType.UNKNOWN)) { This should be: if (!getStorageDomain().getStorageType.isBlock()) You are duplicating the logic in StorageDomainType. Line 54: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); Line 55: return false; Line 56: } Line 57: Line 76: Line 77: List<String> succeededRefreshLuns = new ArrayList<>(); Line 78: for (Map.Entry<String, Map<Long, List<VDS>>> entry : lunToSize.entrySet()) { Line 79: if (entry.getValue().size() != 1) { Line 80: log.error("Failed to refresh device " + entry.getKey() + " Not all VDS are seeing the same size"); We need more info in this error. We should know here the expected size reported by vdsm when we run getDeviceList. We should print the uuid and name of all hosts that see different size. Also, these errors are hidden - this method will always succeed, even if some luns failed to resize. Line 81: } else{ Line 82: String lunId = entry.getKey(); Line 83: // Call PV resize on SPM Line 84: Long pvSize = resizePV(lunId); Line 124: log.error("Failed to refresh PV " + lunId, e); Line 125: } Line 126: } Line 127: Line 128: private Map<String, Map<Long, List<VDS>>> refreshLunSizeAllVds(List<String> luns) { This method should get the expected size of each lun, since vdsm reported it in getDeviceList (path capacity). So this should call each host, and report the hosts that do not see the expected size. We don't nee to create map of sizes, but create map of luns to failed hosts. Line 129: Map<String, Map<Long, List<VDS>>> lunToSize = new HashMap<>(); Line 130: for (String lun : luns) { Line 131: Map<Long, List<VDS>> lunSizeToVDs = new HashMap<>(); Line 132: lunToSize.put(lun, lunSizeToVDs); Line 138: vdsForSize = new ArrayList<>(); Line 139: lunSizeToVDs.put(size, vdsForSize); Line 140: } Line 141: vdsForSize.add(vds); Line 142: } This will not scale. You call a blocking method for each host and wait for reply. If we have 200 hosts and the roundtrip takes 500 milliseconds, this will take 100 seconds. Should be: for each host: send message for each message: wait for reply Line 143: } Line 144: return lunToSize; Line 145: } Line 146: https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java: Line 519: * @param version Line 520: * Compatibility version to check for. Line 521: * @return <code>true</code> if Refresh LUNs is supported for the given version Line 522: */ Line 523: public static boolean isRefreshLunsSupported(Version version) { Other methods here do not use "is" prefix, and they look nicer. I think we should use "refreshLunSupported" Line 524: return supportedInConfig(ConfigValues.RefreshLunsSupported, version); Line 525: } https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/RefreshDeviceSizeVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/RefreshDeviceSizeVDSCommandParameters.java: Line 11: this.deviceId = deviceId; Line 12: } Line 13: Line 14: public RefreshDeviceSizeVDSCommandParameters() { Line 15: } Why do we need empty constructor? Being able to create an object without a vdsId or deviceId is a bug. Line 16: Line 17: public String getDeviceId() { Line 18: return deviceId; Line 19: } Line 17: public String getDeviceId() { Line 18: return deviceId; Line 19: } Line 20: Line 21: public void setDeviceId(String deviceId) { Do we need to set this parameter? Why not use an immutable object? Line 22: this.deviceId = deviceId; Line 23: } Line 24: Line 25: @Override https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ResizePVStorageDomainVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ResizePVStorageDomainVDSCommandParameters.java: Line 12: Line 13: public ResizePVStorageDomainVDSCommandParameters() { Line 14: } Line 15: Line 16: private String privateDevice; Why define this after the constructors and not before them? And why is this privateDevice, and not device? You have an getter for "device, but a setter for "privateDevice", why? Line 17: Line 18: public String getDevice() { Line 19: return privateDevice; Line 20: } Line 24: } Line 25: Line 26: @Override Line 27: public String toString() { Line 28: return String.format("%s, deviceList = %s", super.toString(), getDevice()); getDevice returns a string, why "deviceList"? Line 29: } https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainResource.java: Line 166: /** Line 167: * Resize the LUNs if the user specified Line 168: * @param incoming Line 169: * @param storageDomain Line 170: * @param storageType This documentation does not add any value Line 171: */ Line 172: private void refreshLunSize(StorageDomain incoming, StorageDomain storageDomain, StorageType storageType) { Line 173: if (incoming.getStorage() == null) { Line 174: // LUNs info was not supplied in the request so no need to check whether to refresh the size Line 174: // LUNs info was not supplied in the request so no need to check whether to refresh the size Line 175: return; Line 176: } Line 177: List<LogicalUnit> incomingLuns = getIncomingLuns(incoming.getStorage()); Line 178: if (!incomingLuns.isEmpty()) { We don't need this check. Better find the luns that need to be resized. If the luns list is empty, we will not find any luns to resize anyway. Line 179: List<LogicalUnit> lunsToResize = new ArrayList<>(); Line 180: for(LogicalUnit logicalUnit: incomingLuns){ Line 181: if(logicalUnit.isSetRefreshSize() && logicalUnit.isRefreshSize()){ Line 182: lunsToResize.add(logicalUnit); Line 177: List<LogicalUnit> incomingLuns = getIncomingLuns(incoming.getStorage()); Line 178: if (!incomingLuns.isEmpty()) { Line 179: List<LogicalUnit> lunsToResize = new ArrayList<>(); Line 180: for(LogicalUnit logicalUnit: incomingLuns){ Line 181: if(logicalUnit.isSetRefreshSize() && logicalUnit.isRefreshSize()){ This is code not very clear. What is "isSetRefreshSize? What is "isRefreshSize"? This should be: if (logicalUnit.needsResize()) lunsToResize.add(logicalUnit); Line 182: lunsToResize.add(logicalUnit); Line 183: } Line 184: } Line 185: if(!lunsToResize.isEmpty()){ https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcIIrsServer.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcIIrsServer.java: Line 367: return new StatusOnlyReturnForXmlRpc(response); Line 368: } Line 369: Line 370: @Override Line 371: public RefreshDeviceSizeMapReturnForXmlRpc resizePVStorageDomain(String sdUUID, String spUUID, String device) { resizePVStorageDomain does not make sense. We cannot replace "extend" with "resizePV". Maybe resizeStorageDomainPV? Line 372: JsonRpcRequest request = Line 373: new RequestBuilder("StorageDomain.resizePV").withParameter("storagedomainID", sdUUID) Line 374: .withParameter("storagepoolID", spUUID) Line 375: .withParameter("guid", device) -- To view, visit https://gerrit.ovirt.org/39318 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c5c6d59087736466ee5e7eb0c77ee9282199c62 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Fred Rolland <froll...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
