Liron Aravot has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 8: (20 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: 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 (forceCompensation = true) 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. I'd add another command - RefreshLunSizeCommand which will take care for one luns and will call it from here. 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 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 it exists. use the storagepool validator. Line 54: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_REFRESH_LUNS_UNSUPPORTED_ACTION); Line 55: } Line 56: Line 57: if (!(checkStorageDomain() && checkStorageDomainStatus(StorageDomainStatus.Active))) { 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 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 domain Line 81: Set<LUNs> lunsFoundInSD = new HashSet<>(); Line 82: Guid sdGuid = storageDomain.getId(); Line 83: Line 84: for (LUNs lun : lunsFromDb) { 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 passed luns using collection utils. if the list is not empty, return an error (you can mention there which luns aren't related) Line 83: Line 84: for (LUNs lun : lunsFromDb) { Line 85: if (lunIds.contains(lun.getLUN_id())) { Line 86: if (sdGuid.equals(lun.getStorageDomainId())) { 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.changeStorageDomainStatusInTransaction 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 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 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 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 changed back by the compensation 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 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 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 Line 181: Line 182: Map<String, LUNs> allLuns = getLuns(); Line 183: Line 184: Map<String, List<VDS>> lunToVds = new HashMap<>(); 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 it 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 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? we need to be able to recover if we run it again and not fail. 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 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 Line 203: Line 204: Integer size; Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); 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 Line 217: @Override Line 218: public boolean eval(VDS vds) { Line 219: return vds.getSpmStatus() == VdsSpmStatus.SPM; Line 220: } 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.businessEntitiesById() Line 229: } Line 230: return lunsMap; Line 231: } Line 232: 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 log that ResizeStorageDomainPV failed Line 260: throw e; Line 261: } Line 262: } Line 263: -- 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 <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: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@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
