Mike Kolesnik has posted comments on this change. Change subject: core: MacPoolManager-->ScopedMacPoolManager ......................................................................
Patch Set 38: (49 comments) 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 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 where you got it from (don't expose implementation details). 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/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. 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 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 I think it can be inlined (or moved to ImportVmCommand) and use getMacPool that is already defined there.. 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 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. 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 where you got it from (don't expose implementation details). 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 separate one. 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 separate one. 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. 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 separate one. 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.macpoolmanager.MacPoolManagerStrategy anyway. 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 separate one. 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.macpoolmanager.MacPoolManagerStrategy anyway. 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. Also no need to extract to variable. 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 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.macpoolmanager.MacPoolManagerStrategy anyway. 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. 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 or ctor) since it's not this class` responsibility to understand where the pool comes from or what pool is used.. 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 separate one. 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.. 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 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.. 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 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 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 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 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. 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 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 separate one. 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. 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. 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. 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... 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 separate one. 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 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. 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 separate one. 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 separate one. 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. 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 the way save the Mockito import 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.macpoolmanager.MacPoolManagerStrategy anyway 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.macpoolmanager.MacPoolManagerStrategy anyway 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.macpoolmanager.MacPoolManagerStrategy anyway 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? I don't see how the old test wouldn't work.. 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.. 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 separate one. 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 separate one. 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: 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
