Freddy Rolland has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 8: (54 comments) https://gerrit.ovirt.org/#/c/39318/8/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: > This new command introduced needs a test, at least for the CDA part Done Line 1: package org.ovirt.engine.core.bll.storage; Line 2: Line 3: import java.util.ArrayList; Line 4: import java.util.HashMap; Line 33: import org.ovirt.engine.core.utils.linq.Predicate; Line 34: import org.ovirt.engine.core.utils.transaction.TransactionMethod; Line 35: import org.slf4j.Logger; Line 36: import org.slf4j.LoggerFactory; Line 37: > add @NonTransactiveCommand annotation and if you use compensation add (forc Done Line 38: public class RefreshLunsSizeCommand<T extends ExtendSANStorageDomainParameters> extends Line 39: StorageDomainCommandBase<T> { Line 40: Line 41: private static final Logger log = LoggerFactory.getLogger(RefreshLunsSizeCommand.class); Line 35: import org.slf4j.Logger; Line 36: import org.slf4j.LoggerFactory; Line 37: Line 38: public class RefreshLunsSizeCommand<T extends ExtendSANStorageDomainParameters> extends Line 39: StorageDomainCommandBase<T> { > I think that this command is bit too complicated with all the loops. We simplified the flow . Now we have one loop and one call to SPM. Line 40: Line 41: private static final Logger log = LoggerFactory.getLogger(RefreshLunsSizeCommand.class); Line 42: Line 43: public RefreshLunsSizeCommand(T parameters, CommandContext commandContext) { Line 37: Line 38: public class RefreshLunsSizeCommand<T extends ExtendSANStorageDomainParameters> extends Line 39: StorageDomainCommandBase<T> { Line 40: Line 41: private static final Logger log = LoggerFactory.getLogger(RefreshLunsSizeCommand.class); > you have a logger already defined higher in the hirerchy, can be removed Done Line 42: Line 43: public RefreshLunsSizeCommand(T parameters, CommandContext commandContext) { Line 44: super(parameters, commandContext); Line 45: } Line 49: } Line 50: Line 51: @Override Line 52: protected boolean canDoAction() { Line 53: if (!FeatureSupported.refreshLunSupported(getStoragePool().getCompatibilityVersion())) { > if the storage pool is null we'll have an NPE - first we need to check that Done Line 54: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_REFRESH_LUNS_UNSUPPORTED_ACTION); Line 55: } Line 56: Line 57: if (!(checkStorageDomain() && checkStorageDomainStatus(StorageDomainStatus.Active))) { Line 54: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_REFRESH_LUNS_UNSUPPORTED_ACTION); Line 55: } Line 56: Line 57: if (!(checkStorageDomain() && checkStorageDomainStatus(StorageDomainStatus.Active))) { Line 58: return false; > No message here? The messages are added in the implementation of the methods checkStorageDomain and checkStorageDomainStatus Line 59: } Line 60: Line 61: if (!getStorageDomain().getStorageType().isBlockDomain()) { Line 62: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); Line 58: return false; Line 59: } Line 60: Line 61: if (!getStorageDomain().getStorageType().isBlockDomain()) { Line 62: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); > You may also want to use failCanDoAction Done Line 63: return false; Line 64: } Line 65: Line 66: if (!checkStorageDomainStatusNotEqual(StorageDomainStatus.Locked)) { Line 62: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); Line 63: return false; Line 64: } Line 65: Line 66: if (!checkStorageDomainStatusNotEqual(StorageDomainStatus.Locked)) { > This part seems redundant, you already verified before that the status is a Done Line 67: return false; Line 68: } Line 69: Line 70: if (!checkLunsInStorageDomain(getParameters().getLunIds(), getStorageDomain())) { Line 63: return false; Line 64: } Line 65: Line 66: if (!checkStorageDomainStatusNotEqual(StorageDomainStatus.Locked)) { Line 67: return false; > If this condition still valid, do you want to add a msg? I removed this check Line 68: } Line 69: Line 70: if (!checkLunsInStorageDomain(getParameters().getLunIds(), getStorageDomain())) { Line 71: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_LUNS_NOT_PART_OF_STORAGE_DOMAIN); Line 67: return false; Line 68: } Line 69: Line 70: if (!checkLunsInStorageDomain(getParameters().getLunIds(), getStorageDomain())) { Line 71: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_LUNS_NOT_PART_OF_STORAGE_DOMAIN); > Same as before - failCanDoAction Done Line 72: return false; Line 73: } Line 74: Line 75: return true; Line 74: Line 75: return true; Line 76: } Line 77: Line 78: private boolean checkLunsInStorageDomain(ArrayList<String> lunIds, StorageDomain storageDomain) { > change the arraylist to collection or list Done Line 79: // Get LUNs from DB Line 80: List<LUNs> lunsFromDb = getLunDao().getAll(); Line 81: Set<LUNs> lunsFoundInSD = new HashSet<>(); Line 82: Guid sdGuid = storageDomain.getId(); Line 76: } Line 77: Line 78: private boolean checkLunsInStorageDomain(ArrayList<String> lunIds, StorageDomain storageDomain) { Line 79: // Get LUNs from DB Line 80: List<LUNs> lunsFromDb = getLunDao().getAll(); > no need to get all the luns on the system, get the one's relevant for the d Done Line 81: Set<LUNs> lunsFoundInSD = new HashSet<>(); Line 82: Guid sdGuid = storageDomain.getId(); Line 83: Line 84: for (LUNs lun : lunsFromDb) { Line 77: Line 78: private boolean checkLunsInStorageDomain(ArrayList<String> lunIds, StorageDomain storageDomain) { Line 79: // Get LUNs from DB Line 80: List<LUNs> lunsFromDb = getLunDao().getAll(); Line 81: Set<LUNs> lunsFoundInSD = new HashSet<>(); > I see that this list is only used to count number of LUNs founf in SD. Mayb I simplified this method according to Liron comment Line 82: Guid sdGuid = storageDomain.getId(); Line 83: Line 84: for (LUNs lun : lunsFromDb) { Line 85: if (lunIds.contains(lun.getLUN_id())) { Line 78: private boolean checkLunsInStorageDomain(ArrayList<String> lunIds, StorageDomain storageDomain) { Line 79: // Get LUNs from DB Line 80: List<LUNs> lunsFromDb = getLunDao().getAll(); Line 81: Set<LUNs> lunsFoundInSD = new HashSet<>(); Line 82: Guid sdGuid = storageDomain.getId(); > i'd suggest to simplify the logic here-substract the domain luns from the p Done Line 83: Line 84: for (LUNs lun : lunsFromDb) { Line 85: if (lunIds.contains(lun.getLUN_id())) { Line 86: if (sdGuid.equals(lun.getStorageDomainId())) { Line 82: Guid sdGuid = storageDomain.getId(); Line 83: Line 84: for (LUNs lun : lunsFromDb) { Line 85: if (lunIds.contains(lun.getLUN_id())) { Line 86: if (sdGuid.equals(lun.getStorageDomainId())) { > These if conditions could be combined Done Line 87: // LUN is part of the storage domain Line 88: lunsFoundInSD.add(lun); Line 89: } Line 90: } Line 84: for (LUNs lun : lunsFromDb) { Line 85: if (lunIds.contains(lun.getLUN_id())) { Line 86: if (sdGuid.equals(lun.getStorageDomainId())) { Line 87: // LUN is part of the storage domain Line 88: lunsFoundInSD.add(lun); > Might as well return false if you find any LUNs in the wrong SD I changed the logic to get luns only from the specific SD Line 89: } Line 90: } Line 91: } Line 92: return lunsFoundInSD.size() == lunIds.size(); Line 93: } Line 94: Line 95: @Override Line 96: protected void executeCommand() { Line 97: executeInNewTransaction(new TransactionMethod<Void>() { > Since you extracted the status change to active to a methods I'd extract th I used StorageDomainCommandBase.changeStorageDomainStatusInTransaction according to Liron comment. Line 98: public Void runInTransaction() { Line 99: setStorageDomainStatus(StorageDomainStatus.Locked, getCompensationContext()); Line 100: getCompensationContext().stateChanged(); Line 101: return null; Line 94: Line 95: @Override Line 96: protected void executeCommand() { Line 97: executeInNewTransaction(new TransactionMethod<Void>() { Line 98: public Void runInTransaction() { > please replace with usage of StorageDomainCommandBase.changeStorageDomainSt Done Line 99: setStorageDomainStatus(StorageDomainStatus.Locked, getCompensationContext()); Line 100: getCompensationContext().stateChanged(); Line 101: return null; Line 102: } Line 101: return null; Line 102: } Line 103: }); Line 104: Line 105: setSucceeded(false); > it's false by default Done Line 106: Line 107: // Call Refresh Device on all Hosts Line 108: List<String> lunsToRefresh = getParameters().getLunIds(); Line 109: Map<String, List<VDS>> lunToFailedVDS = refreshLunSizeAllVds(lunsToRefresh); Line 108: List<String> lunsToRefresh = getParameters().getLunIds(); Line 109: Map<String, List<VDS>> lunToFailedVDS = refreshLunSizeAllVds(lunsToRefresh); Line 110: Line 111: if (!lunToFailedVDS.isEmpty()) { Line 112: ArrayList<String> failedVds = new ArrayList<>(); > define as list Done Line 113: for (Map.Entry<String, List<VDS>> entry : lunToFailedVDS.entrySet()) { Line 114: String lunId = entry.getKey(); Line 115: List<VDS> vdsList = entry.getValue(); Line 116: log.error("Failed to refresh device " + lunId + " Not all VDS are seeing the same size " + Line 112: ArrayList<String> failedVds = new ArrayList<>(); Line 113: for (Map.Entry<String, List<VDS>> entry : lunToFailedVDS.entrySet()) { Line 114: String lunId = entry.getKey(); Line 115: List<VDS> vdsList = entry.getValue(); Line 116: log.error("Failed to refresh device " + lunId + " Not all VDS are seeing the same size " + > Might be worth to add an audit log for that Done Line 117: "VDS :" + vdsList); Line 118: String vdsListString = StringUtils.join(getVdsNameList(vdsList), ", "); //$NON-NLS-1$ Line 119: failedVds.add("LUN :" + lunId + "VDS: " + vdsListString); Line 120: } Line 115: List<VDS> vdsList = entry.getValue(); Line 116: log.error("Failed to refresh device " + lunId + " Not all VDS are seeing the same size " + Line 117: "VDS :" + vdsList); Line 118: String vdsListString = StringUtils.join(getVdsNameList(vdsList), ", "); //$NON-NLS-1$ Line 119: failedVds.add("LUN :" + lunId + "VDS: " + vdsListString); > Spacing, something like: "LUN: " + lunId + " VDS: " + vdsListString Done Line 120: } Line 121: Line 122: changeStorageDomainToActive(); Line 123: throw new VdcBLLException(VdcBllErrors.REFRESH_LUN_ERROR, Line 118: String vdsListString = StringUtils.join(getVdsNameList(vdsList), ", "); //$NON-NLS-1$ Line 119: failedVds.add("LUN :" + lunId + "VDS: " + vdsListString); Line 120: } Line 121: Line 122: changeStorageDomainToActive(); > in that case it's unneeded as you throw an exception, the status will be ch Done Line 123: throw new VdcBLLException(VdcBllErrors.REFRESH_LUN_ERROR, Line 124: "Failed to refresh LUNs. Not all VDS are seeing the same size " + failedVds); Line 125: } Line 126: Line 120: } Line 121: Line 122: changeStorageDomainToActive(); Line 123: throw new VdcBLLException(VdcBllErrors.REFRESH_LUN_ERROR, Line 124: "Failed to refresh LUNs. Not all VDS are seeing the same size " + failedVds); > Add a colon? "Failed [...] size: " Done Line 125: } Line 126: Line 127: // Call PVs resize on SPM Line 128: resizePVs(lunsToRefresh); Line 171: VDSCommandType.RefreshPV, Line 172: new RefreshDeviceSizeVDSCommandParameters(vds.getId(), Line 173: lunId, getStorageDomainId())).getReturnValue(); Line 174: } catch (VdcBLLException e) { Line 175: log.error("Failed to refresh PV " + lunId, e); > it's clear from the error what failed, this catch/log can be removed Done Line 176: throw e; Line 177: } Line 178: } Line 179: Line 176: throw e; Line 177: } Line 178: } Line 179: Line 180: private Map<String, List<VDS>> refreshLunSizeAllVds(List<String> luns) { > please document what's the purpose of this method Done Line 181: Line 182: Map<String, LUNs> allLuns = getLuns(); Line 183: Line 184: Map<String, List<VDS>> lunToVds = new HashMap<>(); Line 177: } Line 178: } Line 179: Line 180: private Map<String, List<VDS>> refreshLunSizeAllVds(List<String> luns) { Line 181: > Please remove empty line Done Line 182: Map<String, LUNs> allLuns = getLuns(); Line 183: Line 184: Map<String, List<VDS>> lunToVds = new HashMap<>(); Line 185: for (String lun : luns) { Line 178: } Line 179: Line 180: private Map<String, List<VDS>> refreshLunSizeAllVds(List<String> luns) { Line 181: Line 182: Map<String, LUNs> allLuns = getLuns(); > this should be named /s/LunsFromChoosenHost and the spm should be passed to This code has been removed Line 183: Line 184: Map<String, List<VDS>> lunToVds = new HashMap<>(); Line 185: for (String lun : luns) { Line 186: int actualSize = getSize(allLuns, lun); Line 179: Line 180: private Map<String, List<VDS>> refreshLunSizeAllVds(List<String> luns) { Line 181: Line 182: Map<String, LUNs> allLuns = getLuns(); Line 183: > Please remove empty line Done Line 184: Map<String, List<VDS>> lunToVds = new HashMap<>(); Line 185: for (String lun : luns) { Line 186: int actualSize = getSize(allLuns, lun); Line 187: for (VDS vds : getAllRunningVdssInPool()) { Line 182: Map<String, LUNs> allLuns = getLuns(); Line 183: Line 184: Map<String, List<VDS>> lunToVds = new HashMap<>(); Line 185: for (String lun : luns) { Line 186: int actualSize = getSize(allLuns, lun); > what happens if the command failed during a previous run? I don't see any problem in the current patch Line 187: for (VDS vds : getAllRunningVdssInPool()) { Line 188: int size = (int) (refreshLun(vds, lun) / SizeConverter.BYTES_IN_GB); Line 189: if (size != actualSize) { Line 190: List<VDS> vdsForSize = lunToVds.get(lun); Line 186: int actualSize = getSize(allLuns, lun); Line 187: for (VDS vds : getAllRunningVdssInPool()) { Line 188: int size = (int) (refreshLun(vds, lun) / SizeConverter.BYTES_IN_GB); Line 189: if (size != actualSize) { Line 190: List<VDS> vdsForSize = lunToVds.get(lun); > /s/vdsForSize/vdsWithDifferentSize Removed this code Line 191: if (vdsForSize == null) { Line 192: vdsForSize = new ArrayList<>(); Line 193: lunToVds.put(lun, vdsForSize); Line 194: } Line 198: } Line 199: return lunToVds; Line 200: } Line 201: Line 202: private Integer getSize(Map<String, LUNs> allLuns, String lunId) { > this method should just get a lun, it doesn't need the map I removed this method Line 203: Line 204: Integer size; Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); Line 198: } Line 199: return lunToVds; Line 200: } Line 201: Line 202: private Integer getSize(Map<String, LUNs> allLuns, String lunId) { > Although it's private I'd refrain from using a method name like getSize as I removed this method Line 203: Line 204: Integer size; Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); Line 200: } Line 201: Line 202: private Integer getSize(Map<String, LUNs> allLuns, String lunId) { Line 203: Line 204: Integer size; > Any reason to prefer Integer to int for this method? I removed this method Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); Line 207: if(!pathsCapacity.isEmpty()){ Line 208: size = pathsCapacity.values().iterator().next(); Line 203: Line 204: Integer size; Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); Line 207: if(!pathsCapacity.isEmpty()){ > Similar to Tal's comment on 209, missing space after if I removed this method Line 208: size = pathsCapacity.values().iterator().next(); Line 209: }else { Line 210: size = lun.getDeviceSize(); Line 211: } Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); Line 207: if(!pathsCapacity.isEmpty()){ Line 208: size = pathsCapacity.values().iterator().next(); Line 209: }else { > A space is missing between the left bracket and the else I removed this method Line 210: size = lun.getDeviceSize(); Line 211: } Line 212: return size; Line 213: } Line 212: return size; Line 213: } Line 214: Line 215: private Map<String, LUNs> getLuns() { Line 216: VDS spmVds = LinqUtils.first(LinqUtils.filter(getAllRunningVdssInPool(), new Predicate<VDS>() { > please export this to a method and cache the spm, it's repeated few times No cache needed on next patch , this code is used only once Line 217: @Override Line 218: public boolean eval(VDS vds) { Line 219: return vds.getSpmStatus() == VdsSpmStatus.SPM; Line 220: } Line 222: GetDeviceListVDSCommandParameters parameters = Line 223: new GetDeviceListVDSCommandParameters(spmVds.getId(), getStorageDomain().getStorageType()); Line 224: Line 225: Map<String, LUNs> lunsMap = new HashMap<>(); Line 226: List<LUNs> luns = (List<LUNs>) runVdsCommand(VDSCommandType.GetDeviceList, parameters).getReturnValue(); > I usually see code doing a vdsReturnValue.getSucceeded() check before the r I agree with you that the API is confusing. In general, an exception will be thrown if the VDSM fails, and the user will eventually see the reason for the failure. Line 227: for (LUNs lun : luns) { Line 228: lunsMap.put(lun.getLUN_id(), lun); Line 229: } Line 230: return lunsMap; Line 224: Line 225: Map<String, LUNs> lunsMap = new HashMap<>(); Line 226: List<LUNs> luns = (List<LUNs>) runVdsCommand(VDSCommandType.GetDeviceList, parameters).getReturnValue(); Line 227: for (LUNs lun : luns) { Line 228: lunsMap.put(lun.getLUN_id(), lun); > instead of creating and populating the map, use Entities.businessEntitiesBy This code has been removed Line 229: } Line 230: return lunsMap; Line 231: } Line 232: Line 244: Line 245: private void resizePVs(List<String> lunsToRefresh) { Line 246: for (String lun : lunsToRefresh) { Line 247: Long pvSize = resizePV(lun); Line 248: log.debug("PV size after resize " + lun + " :" + pvSize); > Spacing/wording nits, maybe "PV size after resize of LUN " + lun + ": " + p Done Line 249: } Line 250: } Line 251: Line 252: private Long resizePV(String lunId) { Line 255: VDSCommandType.ResizeStorageDomainPV, Line 256: new ResizeStorageDomainPVVDSCommandParameters(getStoragePoolId(), Line 257: getStorageDomainId(), lunId)).getReturnValue(); Line 258: } catch (VdcBLLException e) { Line 259: log.error("Failed to resize PV " + lunId, e); > the log doesn't add info to the log, that's unneeded, it's clear from the l Done Line 260: throw e; Line 261: } Line 262: } Line 263: Line 266: if (returnValueUpdatedStorageDomain.getSucceeded()) { Line 267: StorageDomain updatedStorageDomain = Line 268: (StorageDomain) returnValueUpdatedStorageDomain.getReturnValue(); Line 269: updateStorageDomain(updatedStorageDomain); Line 270: } > Do you need an else clause, or does the code expect an exception from runVd I remove the check here. Line 271: } Line 272: Line 273: protected VDSReturnValue getStatsForDomain() { Line 274: VDS spmVds = LinqUtils.first(LinqUtils.filter(getAllRunningVdssInPool(), new Predicate<VDS>() { https://gerrit.ovirt.org/#/c/39318/8/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 12: this.deviceId = deviceId; Line 13: this.storageDomainId = storageDomainId; Line 14: } Line 15: Line 16: public RefreshDeviceSizeVDSCommandParameters() { > This can be private (since it can't be removed) I deleted this class Line 17: } Line 18: Line 19: public String getDeviceId() { Line 20: return deviceId; Line 24: return storageDomainId; Line 25: } Line 26: Line 27: @Override Line 28: public String toString() { > Let's use the "protected ToStringBuilder appendAttributes(ToStringBuilder t I deleted this class Line 29: return String.format("%s,storageDomainId = %s deviceId=%s", Line 30: super.toString(), Line 31: getStorageDomainId(), Line 32: getDeviceId()); https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ResizeStorageDomainPVVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ResizeStorageDomainPVVDSCommandParameters.java: Line 12: setStorageDomainId(storageDomainId); Line 13: this.device = device; Line 14: } Line 15: Line 16: public ResizeStorageDomainPVVDSCommandParameters() { > Similar to other Parameters, private? Empty constructor is public in all other "parameters" classes Line 17: } Line 18: Line 19: public String getDevice() { Line 20: return device; Line 20: return device; Line 21: } Line 22: Line 23: @Override Line 24: public String toString() { > Unfortunately I don't know if ToStringBuilder can be used here yet Leaving at is.. Line 25: return String.format("%s, device = %s", super.toString(), getDevice()); Line 26: } https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 684: succeeded > For consistency with the other messages here, I'd suggest to add a "." at t Done https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties: Line 63: job.DeactivateStorageDomain=Deactivating Storage Domain ${Storage} in Data Center ${StoragePool} Line 64: job.DeactivateStorageDomainWithOvfUpdate=Deactivating Storage Domain ${Storage} in Data Center ${StoragePool}. Line 65: job.AddSANStorageDomain=Adding SAN Storage Domain ${Storage} Line 66: job.ExtendSANStorageDomain=Extending SAN Storage Domain ${Storage} Line 67: job.RefreshLunsSize=Refresh LUNs Size in Domain ${Storage} > For clarity (though technically more than 1 LUN), let's go with "Refreshing Done Line 68: job.RecoveryStoragePool=Recovering Data Center ${StoragePool} Line 69: job.RemoveStoragePool=Removing Data Center ${StoragePool} Line 70: job.UpdateStoragePool=Editing Data Center ${StoragePool} properties Line 71: job.AddExistingFileStorageDomain=Adding File Storage Domain ${Storage} https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties: Line 324: ResourceDoesNotExist=Resource does not exist Line 325: InvalidResourceName=Invalid resource name Line 326: VolumeNotSparse=Volume type is not sparse Line 327: CannotSparsifyVolume=Cannot run virt-sparsify on volume Line 328: CouldNotResizePhysicalVolume=Could Not Resize Physical Volume > No need to capitalize all words, eg "Could not resize Physical Volume" Done Line 329: # Gluster VDSM errors Line 330: GlusterGeneralException=Gluster General Exception Line 331: GlusterPermissionDeniedException=Permission denied Line 332: GlusterSyntaxErrorException=Syntax error https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/StorageDomainResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/StorageDomainResource.java: Line 77: refreshLuns > Can you please add a test for this method in BackendStorageDomainResourceTe Done https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 2656: needs_resize > This new property should be removed. Done https://gerrit.ovirt.org/#/c/39318/8/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 131: public Response refreshLuns(Action action) { Line 132: QueryIdResolver<Guid> storageDomainResolver = Line 133: new QueryIdResolver<Guid>(VdcQueryType.GetStorageDomainById, IdQueryParameters.class); Line 134: org.ovirt.engine.core.common.businessentities.StorageDomain entity = getEntity(storageDomainResolver, true); Line 135: StorageDomain model = map(entity, new StorageDomain()); > I suggest using get() instead of the above lines Done Line 136: StorageType storageType = entity.getStorageType(); Line 137: if (storageType != null) { Line 138: switch (storageType) { Line 139: case ISCSI: https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/GetDeviceListVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/GetDeviceListVDSCommand.java: Line 101: (int) (size / SizeConverter.BYTES_IN_GB)); > you can use SizeConverter.convert instead Done https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/RefreshDeviceSizeMapReturnForXmlRpc.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/RefreshDeviceSizeMapReturnForXmlRpc.java: Line 14: SIZE > there's already a constant in VdsProperties.size This class has been deleted -- 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: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Fred Rolland <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
