Martin Mucha has posted comments on this change. Change subject: core: MacPoolManager-->ScopedMacPoolManager ......................................................................
Patch Set 38: (50 comments) answers. http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 889: getCompensationContext().snapshotNewEntity(iface.getStatistics()); Line 890: } Line 891: } Line 892: Line 893: private MacPoolManagerStrategy getMacPool() { > This is unused Done Line 894: return MacPoolPerDc.getInstance().poolForDataCenter(getStoragePoolId()); Line 895: } Line 896: Line 897: protected void addVmInit() { http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: Line 2147: ExecutionContext executionContext) { Line 2148: return Backend.getInstance().runInternalMultipleActions(actionType, parameters, executionContext); Line 2149: } Line 2150: Line 2151: protected MacPoolManagerStrategy getMacPoolForDataCenter() { > This should be named simply getMacPool since the command shouldn't care whe Done Line 2152: return MacPoolPerDc.getInstance().poolForDataCenter(getStoragePoolId()); Line 2153: } http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java: Line 303: return checkFreeSpaceAndTypeOnDestDomains(); Line 304: } Line 305: Line 306: protected boolean verifyAddVM() { Line 307: final MacPoolManagerStrategy macPool = getMacPoolForVm(); > This generates a null pointer exception when in MacPoolPerDc.poolForVm() wh replaced with getMacPool() method. Line 308: final List<String> reasons = getReturnValue().getCanDoActionMessages(); Line 309: final int nicsCount = getParameters().getVmsCount() Line 310: * getVmNicDao().getAllForTemplate(getVmTemplateId()).size(); Line 311: final int priority = getParameters().getVmStaticData().getPriority(); http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java: Line 106: private List<DiskImage> imageList; Line 107: Line 108: private final List<String> macsAdded = new ArrayList<String>(); Line 109: private final SnapshotsManager snapshotsManager = new SnapshotsManager(); Line 110: protected MacPoolManagerStrategy macPool; > As far as I can see this field isn't used, so you should delete it. Done Line 111: Line 112: public ImportVmCommand(T parameters) { Line 113: super(parameters); Line 114: } Line 195: Line 196: if (getParameters().isImportAsNewEntity()) { Line 197: initImportClonedVm(); Line 198: Line 199: final VM vm = getVm(); > No need to extract to variable Done Line 200: if (vm.getInterfaces().size() > getMacPoolForVm().getAvailableMacsCount()) { Line 201: addCanDoActionMessage(VdcBllMessages.MAC_POOL_NOT_ENOUGH_MAC_ADDRESSES); Line 202: return false; Line 203: } http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java: Line 387: } Line 388: return permissionCheckSubject; Line 389: } Line 390: Line 391: protected void fillMacAddressIfMissing(VmNic iface) { > I see this is only called from ImportVmCommand replaced with getMacPool() Line 392: final MacPoolManagerStrategy pool = getMacPoolForVmNic(iface); Line 393: if (StringUtils.isEmpty(iface.getMacAddress()) Line 394: && (pool.getAvailableMacsCount() >= 1)) { Line 395: iface.setMacAddress(pool.allocateNewMac()); Line 422: protected MacPoolManagerStrategy getMacPoolForVmNic(VmNic iface) { Line 423: return MacPoolPerDc.getInstance().poolForVmNic(iface); Line 424: } Line 425: Line 426: protected MacPoolManagerStrategy getMacPoolForDataCenter() { > This is already available from CommandBase Done Line 427: return MacPoolPerDc.getInstance().poolForDataCenter(getParameters().getStoragePoolId()); Line 428: } Line 429: Line 430: /** http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java: Line 228: } Line 229: Line 230: protected void removeVmNetwork() { Line 231: if (getInterfaces() != null) { Line 232: final MacPoolManagerStrategy macPool = getMacPoolForVm(); > I wouldn't define this outside the loop, it has no real added value. Done Line 233: for (VmNic iface : getInterfaces()) { Line 234: macPool.freeMac(iface.getMacAddress()); Line 235: } Line 236: } Line 235: } Line 236: } Line 237: } Line 238: Line 239: protected MacPoolManagerStrategy getMacPoolForVm() { > This should be named simply getMacPool since the command shouldn't care whe replace with getMacPool Line 240: return MacPoolPerDc.getInstance().poolForVm(getVm()); Line 241: } Line 242: Line 243: protected Set<String> removeVmSnapshots() { Line 541: return returnValue; Line 542: } Line 543: Line 544: protected boolean canRunActionOnNonManagedVm() { Line 545: ValidationResult nonManagedVmValidationResult = VmHandler.canRunActionOnNonManagedVm(getVm(), getActionType()); > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 546: if (!nonManagedVmValidationResult.isValid()) { Line 547: return failCanDoAction(nonManagedVmValidationResult.getMessage()); Line 548: } Line 549: return true; http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java: Line 77: /** Line 78: * Initialize static list containers, for identity and permission check. The initialization should be executed Line 79: * before calling ObjectIdentityChecker. Line 80: * Line 81: * @see Backend#initHandlers > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 82: */ Line 83: public static void init() { Line 84: Class<?>[] inspectedClassNames = new Class<?>[] { Line 85: VmBase.class, Line 145: * @return Line 146: */ Line 147: public static boolean verifyAddVm(List<String> reasons, Line 148: int nicsCount, Line 149: int vmPriority, MacPoolManagerStrategy macPool) { > As per the formatting, this addition should be in a separate line. Done Line 150: boolean returnValue = true; Line 151: if (macPool.getAvailableMacsCount() < nicsCount) { Line 152: if (reasons != null) { Line 153: reasons.add(VdcBllMessages.MAC_POOL_NOT_ENOUGH_MAC_ADDRESSES.toString()); Line 400: */ Line 401: public static void updateVmGuestAgentVersion(final VM vm) { Line 402: if (vm.getAppList() != null) { Line 403: final String[] parts = vm.getAppList().split("[,]", -1); Line 404: if (parts.length != 0) { > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 405: final List<String> possibleAgentAppNames = Config.<List<String>> getValue(ConfigValues.AgentAppName); Line 406: final Map<String, String> spiceDriversInGuest = Line 407: Config.<Map<String, String>> getValue(ConfigValues.SpiceDriverNameInGuest); Line 408: final String spiceDriverInGuest = http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java: Line 44: private Log log = LogFactory.getLog(getClass()); Line 45: Line 46: /** Line 47: * Add a {@link VmNic} to the VM. Allocates a MAC from the Line 48: * {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy} if necessary, otherwise, if > No need for FQCN since you're importing org.ovirt.engine.core.bll.network.m Done Line 49: * {@code ConfigValues.HotPlugEnabled} is true, forces adding the MAC address to the Line 50: * {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy}. If Line 51: * HotPlug is not enabled tries to add the {@link VmNic}'s MAC address to the Line 52: * {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy}, and throws a Line 49: * {@code ConfigValues.HotPlugEnabled} is true, forces adding the MAC address to the Line 50: * {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy}. If Line 51: * HotPlug is not enabled tries to add the {@link VmNic}'s MAC address to the Line 52: * {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy}, and throws a Line 53: * {@link VdcBLLException} if it fails. > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 54: * Line 55: * @param iface Line 56: * The interface to save. Line 57: * @param compensationContext Line 56: * The interface to save. Line 57: * @param compensationContext Line 58: * Used to snapshot the saved entities. Line 59: * @param reserveExistingMac Line 60: * Used to denote if we want to reserve the NIC's MAC address in the {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy} > No need for FQCN since you're importing org.ovirt.engine.core.bll.network.m Done Line 61: * @param clusterCompatibilityVersion Line 62: * the compatibility version of the cluster Line 63: * @return <code>true</code> if the MAC wasn't used, <code>false</code> if it was. Line 64: */ Line 68: int osId, Line 69: Version clusterCompatibilityVersion) { Line 70: Line 71: if (reserveExistingMac) { Line 72: final MacPoolManagerStrategy macPoolManager = getMacPoolManager(iface.getVmId()); > Should be named just macPool. Done Line 73: final String ifaceMacAddress = iface.getMacAddress(); Line 74: Line 75: if (FeatureSupported.hotPlug(clusterCompatibilityVersion) Line 76: && getOsRepository().hasNicHotplugSupport(osId, clusterCompatibilityVersion)) { Line 69: Version clusterCompatibilityVersion) { Line 70: Line 71: if (reserveExistingMac) { Line 72: final MacPoolManagerStrategy macPoolManager = getMacPoolManager(iface.getVmId()); Line 73: final String ifaceMacAddress = iface.getMacAddress(); > There's no real value to extract this to variable Done Line 74: Line 75: if (FeatureSupported.hotPlug(clusterCompatibilityVersion) Line 76: && getOsRepository().hasNicHotplugSupport(osId, clusterCompatibilityVersion)) { Line 77: macPoolManager.forceAddMac(ifaceMacAddress); Line 119: }); Line 120: } Line 121: Line 122: /** Line 123: * Remove all {@link VmNic}s from the VM, and remove the Mac addresses from {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy}. > No need for FQCN since you're importing org.ovirt.engine.core.bll.network.m Done Line 124: * Line 125: * @param vmId Line 126: * The ID of the VM to remove from. Line 127: */ Line 129: List<VmNic> interfaces = getVmNicDao().getAllForVm(vmId); Line 130: if (interfaces != null) { Line 131: removeFromExternalNetworks(interfaces); Line 132: Line 133: final MacPoolManagerStrategy macPool = getMacPoolManager(vmId); > No need to extract to variable. Done Line 134: for (VmNic iface : interfaces) { Line 135: macPool.freeMac(iface.getMacAddress()); Line 136: getVmNicDao().remove(iface.getId()); Line 137: getVmNetworkStatisticsDao().remove(iface.getId()); Line 233: AuditLogDirector.log(logable, auditLogType); Line 234: } Line 235: Line 236: protected MacPoolManagerStrategy getMacPoolManager(Guid vmId) { Line 237: return MacPoolPerDc.getInstance().poolForVm(vmId); > It would make sense to inject the MacPool to this class (either via getter Done Line 238: } Line 239: Line 240: protected VmNetworkStatisticsDao getVmNetworkStatisticsDao() { Line 241: return DbFacade.getInstance().getVmNetworkStatisticsDao(); http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/macpoolmanager/ObjectCounter.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/macpoolmanager/ObjectCounter.java: Line 45: * decrements number of its occurrences, removing instance if possible(count reaches zero). Line 46: * Line 47: * @param key instance to remove. Line 48: * Line 49: * @return true if instance was removed && count decremented. > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 50: */ Line 51: public void remove(T key) { Line 52: Counter counter = map.get(key); Line 53: if (counter == null) { http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/macpoolmanager/Range.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/macpoolmanager/Range.java: Line 48: if (!usedMacs.get(arrayIndex)) { Line 49: availableMacsCount--; Line 50: usedMacs.set(arrayIndex, true); Line 51: return true; Line 52: > No need to add newline.. Done Line 53: } Line 54: Line 55: if (allowDuplicates) { Line 56: return macDuplicityCount.add(arrayIndex); http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/AbstractVmInterfaceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/AbstractVmInterfaceCommand.java: Line 54: protected void setActionMessageParameters() { Line 55: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__INTERFACE); Line 56: } Line 57: Line 58: protected boolean addMacToPool(String macAddress, MacPoolManagerStrategy macPool) { > You should use the pool available from the inherited getMacPool function Done Line 59: if (macPool.addMac(macAddress)) { Line 60: return true; Line 61: } else { Line 62: throw new VdcBLLException(VdcBllErrors.MAC_ADDRESS_IS_IN_USE); Line 62: throw new VdcBLLException(VdcBllErrors.MAC_ADDRESS_IS_IN_USE); Line 63: } Line 64: } Line 65: Line 66: protected ValidationResult macAvailable(MacPoolManagerStrategy macPool) { > Same here.. Done Line 67: Boolean allowDupMacs = Config.<Boolean> getValue(ConfigValues.AllowDuplicateMacAddresses); Line 68: return macPool.isMacInUse(getMacAddress()) && !allowDupMacs Line 69: ? new ValidationResult(VdcBllMessages.NETWORK_MAC_ADDRESS_IN_USE) Line 70: : ValidationResult.VALID; http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/AddVmInterfaceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/AddVmInterfaceCommand.java: Line 35: } Line 36: Line 37: @Override Line 38: protected void executeVmCommand() { Line 39: final VmNetworkInterface anInterface = getInterface(); > There is no added value to extract this to a variable Done Line 40: final VmStatic vmStatic = getVmStaticDAO().get(getParameters().getVmId()); Line 41: addCustomValue("InterfaceType", Line 42: (VmInterfaceType.forValue(anInterface.getType()).getDescription()).toString()); Line 43: this.setVmName(vmStatic.getName()); Line 36: Line 37: @Override Line 38: protected void executeVmCommand() { Line 39: final VmNetworkInterface anInterface = getInterface(); Line 40: final VmStatic vmStatic = getVmStaticDAO().get(getParameters().getVmId()); > Should be named just VM Done Line 41: addCustomValue("InterfaceType", Line 42: (VmInterfaceType.forValue(anInterface.getType()).getDescription()).toString()); Line 43: this.setVmName(vmStatic.getName()); Line 44: Line 43: this.setVmName(vmStatic.getName()); Line 44: Line 45: boolean succeeded = false; Line 46: boolean macAddedToPool = false; Line 47: final String macAddress = getMacAddress(); > There is no added value to extract this to a variable Done Line 48: final MacPoolManagerStrategy pool = getMacPoolManager(); Line 49: Line 50: try { Line 51: if (StringUtils.isEmpty(macAddress)) { Line 44: Line 45: boolean succeeded = false; Line 46: boolean macAddedToPool = false; Line 47: final String macAddress = getMacAddress(); Line 48: final MacPoolManagerStrategy pool = getMacPoolManager(); > There is no added value to extract this to a variable Done Line 49: Line 50: try { Line 51: if (StringUtils.isEmpty(macAddress)) { Line 52: Line 82: } Line 83: } Line 84: } Line 85: Line 86: protected MacPoolManagerStrategy getMacPoolManager() { > No need for this method, you can use the one you inherited. Done Line 87: return MacPoolPerDc.getInstance().poolForVm(getVm()); Line 88: } Line 89: Line 90: private void addInterfaceDeviceToDb() { Line 153: if (StringUtils.isNotEmpty(getMacAddress())) { Line 154: if (!validate(macAvailable(getMacPoolManager()))) { Line 155: return false; Line 156: } Line 157: } else if (getMacPoolManager().getAvailableMacsCount() <= 0){ > No need to delete the space between the brackets Done Line 158: addCanDoActionMessage(VdcBllMessages.MAC_POOL_NOT_ENOUGH_MAC_ADDRESSES); Line 159: return false; Line 160: } Line 161: http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/RemoveVmInterfaceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/RemoveVmInterfaceCommand.java: Line 30: } Line 31: Line 32: @Override Line 33: protected void executeVmCommand() { Line 34: setVmName(getVmStaticDAO().get(getParameters().getVmId()).getName()); > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 35: VmNic iface = getVmNicDao().get(getParameters().getInterfaceId()); Line 36: Line 37: if (iface != null) { Line 38: new ExternalNetworkManager(iface).deallocateIfExternal(); Line 61: } Line 62: }); Line 63: } Line 64: Line 65: protected MacPoolManagerStrategy getMacPoolForVmNic(VmNic iface) { > You should use the method you inherited. Done Line 66: return MacPoolPerDc.getInstance().poolForVmNic(iface); Line 67: } Line 68: Line 69: @Override http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/UpdateVmInterfaceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/UpdateVmInterfaceCommand.java: Line 74: (VmInterfaceType.forValue(getInterface().getType()).getDescription()).toString()); Line 75: Line 76: boolean succeeded = false; Line 77: boolean macAddedToPool = false; Line 78: final MacPoolManagerStrategy macPool = getMacPoolForVm(); > There's no real value to define this in a variable. Done Line 79: Line 80: try { Line 81: if (isVnicProfileChanged(oldIface, getInterface())) { Line 82: Network newNetwork = NetworkHelper.getNetworkByVnicProfileId(getInterface().getVnicProfileId()); Line 161: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_EXIST); Line 162: return false; Line 163: } Line 164: Line 165: final MacPoolManagerStrategy macPool = getMacPoolForVm(); > Not sure why you defined this here, in any case it doesn't have any value. Done Line 166: Line 167: if (!canRunActionOnNonManagedVm()) { Line 168: return false; Line 169: } http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStoragePoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStoragePoolCommand.java: Line 87: Line 88: removeDataCenter(); Line 89: Line 90: getMacPoolForDataCenter().freeMacs(macsToRemove); Line 91: MacPoolPerDc.getInstance().removePool(getStoragePoolId()); > Not sure how this line makes any sense... What's the problem? this line is removing pool for given datacenter. It's record was removed from DB, all related MACs was removed from pool, so there's empty pool left, which should also be removed. Line 92: setSucceeded(true); Line 93: } Line 94: Line 95: private void removeDataCenter() { http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHandlingCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHandlingCommandBase.java: Line 581: } Line 582: Line 583: /** @return The supported storage domain formats, delimited by commas (","). */ Line 584: protected String getSupportedStorageFormats(Version version) { Line 585: return Config.getValue(ConfigValues.SupportedStorageFormats, version.toString()); > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 586: } Line 587: Line 588: /* Overidden DAO access methods, for easier testing */ Line 589: http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmCommandTest.java: Line 63: Line 64: @ClassRule Line 65: public static MockConfigRule mcr = new MockConfigRule( Line 66: mockConfig(ConfigValues.VirtIoScsiEnabled, Version.v3_2.toString(), false), Line 67: mockConfig(ConfigValues.MacPoolRanges, "00:1a:4a:15:c0:00-00:1a:4a:15:c0:ff"), > Not sure why you need to mock all these config values Done Line 68: mockConfig(ConfigValues.MaxMacsCountInPool, 100000), Line 69: mockConfig(ConfigValues.AllowDuplicateMacAddresses, false)); Line 70: Line 71: @Rule Line 75: OsRepository osRepository; Line 76: Line 77: @BeforeClass Line 78: public static void setUpBeforeClass() throws Exception { Line 79: ScopedMacPoolManagerUtilTest.initializeWithSoleSharedMacPoolUsedByEveryone(); > This can be accomplished much more simply by spying on the command. Done Line 80: } Line 81: Line 82: @Before Line 83: public void setUp() { Line 210: } Line 211: Line 212: /** Line 213: * Checking that other fields in Line 214: * {@link org.ovirt.engine.core.common.businessentities.VmStatic} > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 215: * don't get validated when importing a VM. Line 216: */ Line 217: @Test Line 218: public void testOtherFieldsNotValidatedInImport() { http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java: Line 57: public class ImportVmTemplateCommandTest { Line 58: Line 59: @ClassRule Line 60: public static MockConfigRule mcr = new MockConfigRule( Line 61: mockConfig(ConfigValues.VirtIoScsiEnabled, Version.v3_2.toString(), false)); > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 62: Line 63: @Before Line 64: public void setUpBeforeClass() throws Exception { Line 65: ScopedMacPoolManagerUtilTest.initializeWithSoleSharedMacPoolUsedByEveryone(); Line 61: mockConfig(ConfigValues.VirtIoScsiEnabled, Version.v3_2.toString(), false)); Line 62: Line 63: @Before Line 64: public void setUpBeforeClass() throws Exception { Line 65: ScopedMacPoolManagerUtilTest.initializeWithSoleSharedMacPoolUsedByEveryone(); > This can be accomplished much more simply by spying on the command. Done Line 66: } Line 67: Line 68: @Test Line 69: public void insufficientDiskSpace() { http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/VmInterfaceManagerTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/VmInterfaceManagerTest.java: Line 89: @SuppressWarnings("unchecked") Line 90: public void setupMocks() { Line 91: MockitoAnnotations.initMocks(this); Line 92: Line 93: doReturn(macPoolManagerStrategy).when(vmInterfaceManager).getMacPoolManager(Mockito.any(Guid.class)); > Mockito.any is already imported statically so you can just use "any" and on Done Line 94: doReturn(vmNetworkStatisticsDAO).when(vmInterfaceManager).getVmNetworkStatisticsDao(); Line 95: doReturn(vmNetworkInterfaceDAO).when(vmInterfaceManager).getVmNetworkInterfaceDao(); Line 96: doReturn(vmNicDao).when(vmInterfaceManager).getVmNicDao(); Line 97: doReturn(vmDAO).when(vmInterfaceManager).getVmDAO(); Line 167: when(vmNetworkInterfaceDAO.getAllForVm(vm.getId())).thenReturn(vm.getInterfaces()); Line 168: } Line 169: Line 170: /** Line 171: * Verify that {@link VmInterfaceManager#add} delegated correctly to {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy} & DAOs. > No need for FQCN since you import org.ovirt.engine.core.bll.network.macpool Done Line 172: * Line 173: * @param iface Line 174: * The interface to check for. Line 175: * @param addMacVerification Line 172: * Line 173: * @param iface Line 174: * The interface to check for. Line 175: * @param addMacVerification Line 176: * Mode to check (times(1), never(), etc) for {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy#addMac(String)}. > No need for FQCN since you import org.ovirt.engine.core.bll.network.macpool Done Line 177: */ Line 178: protected void verifyAddDelegatedCorrectly(VmNic iface, VerificationMode addMacVerification) { Line 179: verify(macPoolManagerStrategy, addMacVerification).forceAddMac(iface.getMacAddress()); Line 180: verify(vmNicDao).save(iface); Line 181: verify(vmNetworkStatisticsDAO).save(iface.getStatistics()); Line 182: } Line 183: Line 184: /** Line 185: * Verify that {@link VmInterfaceManager#removeAll} delegated correctly to {@link org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy} & DAOs. > No need for FQCN since you import org.ovirt.engine.core.bll.network.macpool Done Line 186: * Line 187: * @param iface Line 188: * The interface to check for. Line 189: */ Line 199: protected VmNic createNewInterface() { Line 200: VmNic iface = new VmNic(); Line 201: iface.setId(Guid.newGuid()); Line 202: Line 203: final long from = MacAddressRangeUtils.macToLong("00:1A:4A:01:00:00"); > Not sure, whats the rationale here? Done Line 204: final int MAX_MACS_IN_POOL = 100000; Line 205: Line 206: final int rndInt = RandomUtils.instance().nextInt(MAX_MACS_IN_POOL); Line 207: long macLong = from + rndInt; http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/macpoolmanager/ScopedMacPoolManagerUtilTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/macpoolmanager/ScopedMacPoolManagerUtilTest.java: Line 15: import org.ovirt.engine.core.dal.dbbroker.DbFacadeLocator; Line 16: import org.ovirt.engine.core.dao.MacPoolDao; Line 17: import org.ovirt.engine.core.dao.StoragePoolDAO; Line 18: Line 19: public class ScopedMacPoolManagerUtilTest { > This is not necessary if you simply spy on the commands.. Done Line 20: private ScopedMacPoolManagerUtilTest() { Line 21: } Line 22: Line 23: public static void initializeWithSoleSharedMacPoolUsedByEveryone() { http://gerrit.ovirt.org/#/c/26800/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java: Line 133: return super.getName(); Line 134: } Line 135: Line 136: public Guid getVmtGuid() { Line 137: return vmtGuid; > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 138: } Line 139: Line 140: public void setVmtGuid(Guid value) { Line 141: vmtGuid = value; Line 136: public Guid getVmtGuid() { Line 137: return vmtGuid; Line 138: } Line 139: Line 140: public void setVmtGuid(Guid value) { > Although a nice change, it's unrelated to this patch and should go in a sep Done Line 141: vmtGuid = value; Line 142: } Line 143: Line 144: public boolean isInitialized() { -- To view, visit http://gerrit.ovirt.org/26800 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I779f4d031409fbc422077ddf291fa34f955f3206 Gerrit-PatchSet: 38 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
