Martin Mucha has uploaded a new change for review. Change subject: core: removed constructor from VmInterfacemanager ......................................................................
core: removed constructor from VmInterfacemanager There were two constructors, one with MacPoolManagerStrategy parameter. That led to situation, when object created 'inappropriately' some methods failed with NPE, which is confusing. This means, that this class should be splitted into two (doable, but also confusing), methods which do not need macPool can be turned into static ones (generaly wrong and problematic due to mocking daos here), or macPool field can be passed as argument into methods which needs it. I opted for the last possibility, which removed possibility to instantiate class 'badly' and removed specifying macPool when not needed. More procedural approach isn't great, but probably best of simple fixes. Change-Id: I65aaf5e2c189d2e43e1102b10f621ede0a628aa3 Signed-off-by: Martin Mucha <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/DetachNetworkFromVdsInterfaceCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/ActivateDeactivateVmNicCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/VmInterfaceManagerTest.java 7 files changed, 37 insertions(+), 25 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/73/36573/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java index 16aee42..7344e99 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java @@ -947,7 +947,7 @@ protected void addVmNetwork() { List<VmNic> nics = getVmInterfaces(); - VmInterfaceManager vmInterfaceManager = new VmInterfaceManager(getMacPool()); + VmInterfaceManager vmInterfaceManager = new VmInterfaceManager(); vmInterfaceManager.sortVmNics(nics, getVmInterfaceDevices()); List<String> macAddresses = getMacPool().allocateMacAddresses(nics.size()); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java index 87d51a8..c9540ed 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java @@ -16,6 +16,7 @@ import org.ovirt.engine.core.bll.context.CommandContext; import org.ovirt.engine.core.bll.memory.MemoryUtils; import org.ovirt.engine.core.bll.network.VmInterfaceManager; +import org.ovirt.engine.core.bll.network.macpoolmanager.MacPoolManagerStrategy; import org.ovirt.engine.core.bll.network.vm.VnicProfileHelper; import org.ovirt.engine.core.bll.profiles.CpuProfileHelper; import org.ovirt.engine.core.bll.profiles.DiskProfileHelper; @@ -1135,7 +1136,8 @@ } protected void addVmInterfaces() { - VmInterfaceManager vmInterfaceManager = new VmInterfaceManager(getMacPool()); + MacPoolManagerStrategy macPool = getMacPool(); + VmInterfaceManager vmInterfaceManager = new VmInterfaceManager(); VnicProfileHelper vnicProfileHelper = new VnicProfileHelper(getVdsGroupId(), @@ -1149,7 +1151,7 @@ // If we import it as a new entity, then we allocate all MAC addresses in advance if (getParameters().isImportAsNewEntity()) { - List<String> macAddresses = getMacPool().allocateMacAddresses(nics.size()); + List<String> macAddresses = macPool.allocateMacAddresses(nics.size()); for (int i = 0; i < nics.size(); ++i) { nics.get(i).setMacAddress(macAddresses.get(i)); } @@ -1160,10 +1162,11 @@ vnicProfileHelper.updateNicWithVnicProfileForUser(iface, getCurrentUser()); vmInterfaceManager.add(iface, - getCompensationContext(), - !getParameters().isImportAsNewEntity(), - getVm().getOs(), - getVdsGroup().getcompatibility_version()); + getCompensationContext(), + !getParameters().isImportAsNewEntity(), + getVm().getOs(), + getVdsGroup().getcompatibility_version(), + macPool); macsAdded.add(iface.getMacAddress()); } @@ -1273,7 +1276,7 @@ } protected void removeVmNetworkInterfaces() { - new VmInterfaceManager(getMacPool()).removeAll(getVmId()); + new VmInterfaceManager().removeAll(getVmId(), getMacPool()); } protected void endImportCommand() { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java index bb537f4..a10270d 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java @@ -43,13 +43,8 @@ public class VmInterfaceManager { private Logger log = LoggerFactory.getLogger(getClass()); - private MacPoolManagerStrategy macPool; public VmInterfaceManager() { - } - - public VmInterfaceManager(MacPoolManagerStrategy macPool) { - this.macPool = macPool; } /** @@ -69,13 +64,15 @@ * Used to denote if we want to reserve the NIC's MAC address in the {@link MacPoolManagerStrategy} * @param clusterCompatibilityVersion * the compatibility version of the cluster + * @param macPool macPool used for registering used mac address. * @return <code>true</code> if the MAC wasn't used, <code>false</code> if it was. */ public void add(final VmNic iface, CompensationContext compensationContext, boolean reserveExistingMac, int osId, - Version clusterCompatibilityVersion) { + Version clusterCompatibilityVersion, + MacPoolManagerStrategy macPool) { if (reserveExistingMac) { @@ -131,8 +128,9 @@ * * @param vmId * The ID of the VM to remove from. + * @param macPool MAC pool to release MAC addresses from. */ - public void removeAll(Guid vmId) { + public void removeAll(Guid vmId, MacPoolManagerStrategy macPool) { List<VmNic> interfaces = getVmNicDao().getAllForVm(vmId); if (interfaces != null) { removeFromExternalNetworks(interfaces); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/DetachNetworkFromVdsInterfaceCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/DetachNetworkFromVdsInterfaceCommand.java index 7d23c98..63394a2 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/DetachNetworkFromVdsInterfaceCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/DetachNetworkFromVdsInterfaceCommand.java @@ -138,9 +138,8 @@ } } - List<String> vmNames = - new VmInterfaceManager(getMacPool()).findActiveVmsUsingNetworks(vds.getId(), - Collections.singletonList(getParameters().getNetwork().getName())); + List<String> vmNames = new VmInterfaceManager().findActiveVmsUsingNetworks(vds.getId(), + Collections.singletonList(getParameters().getNetwork().getName())); if (!vmNames.isEmpty()) { addCanDoActionMessage(VdcBllMessages.NETWORK_CANNOT_DETACH_NETWORK_USED_BY_VMS); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/ActivateDeactivateVmNicCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/ActivateDeactivateVmNicCommand.java index 1ff9a3c..4ffc412 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/ActivateDeactivateVmNicCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/ActivateDeactivateVmNicCommand.java @@ -230,7 +230,7 @@ protected ValidationResult macAvailable() { Boolean allowDupMacs = Config.<Boolean> getValue(ConfigValues.AllowDuplicateMacAddresses); - VmInterfaceManager vmInterfaceManager = new VmInterfaceManager(getMacPool()); + VmInterfaceManager vmInterfaceManager = new VmInterfaceManager(); if (allowDupMacs || !vmInterfaceManager.existsPluggedInterfaceWithSameMac(getParameters().getNic())) { return ValidationResult.VALID; } else { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java index 99734d6..ce8dabb 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java @@ -598,14 +598,15 @@ * The user that performs the action */ protected void synchronizeNics(VM vm, CompensationContext compensationContext, DbUser user) { - VmInterfaceManager vmInterfaceManager = new VmInterfaceManager(getMacPool(vm.getStoragePoolId())); + MacPoolManagerStrategy macPool = getMacPool(vm.getStoragePoolId()); + VmInterfaceManager vmInterfaceManager = new VmInterfaceManager(); VnicProfileHelper vnicProfileHelper = new VnicProfileHelper(vm.getVdsGroupId(), vm.getStoragePoolId(), vm.getVdsGroupCompatibilityVersion(), AuditLogType.IMPORTEXPORT_SNAPSHOT_VM_INVALID_INTERFACES); - vmInterfaceManager.removeAll(vm.getId()); + vmInterfaceManager.removeAll(vm.getId(), macPool); for (VmNetworkInterface vmInterface : vm.getInterfaces()) { vmInterface.setVmId(vm.getId()); // These fields might not be saved in the OVF, so fill them with reasonable values. @@ -614,7 +615,12 @@ } vnicProfileHelper.updateNicWithVnicProfileForUser(vmInterface, user); - vmInterfaceManager.add(vmInterface, compensationContext, true, vm.getOs(), vm.getVdsGroupCompatibilityVersion()); + vmInterfaceManager.add(vmInterface, + compensationContext, + true, + vm.getOs(), + vm.getVdsGroupCompatibilityVersion(), + macPool); } vnicProfileHelper.auditInvalidInterfaces(vm.getName()); diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/VmInterfaceManagerTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/VmInterfaceManagerTest.java index 0983379..2dc78ed 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/VmInterfaceManagerTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/VmInterfaceManagerTest.java @@ -81,7 +81,7 @@ @Before @SuppressWarnings("unchecked") public void setupMocks() { - vmInterfaceManager = Mockito.spy(new VmInterfaceManager(macPoolManagerStrategy)); + vmInterfaceManager = Mockito.spy(new VmInterfaceManager()); doReturn(vmNetworkStatisticsDAO).when(vmInterfaceManager).getVmNetworkStatisticsDao(); doReturn(vmNetworkInterfaceDAO).when(vmInterfaceManager).getVmNetworkInterfaceDao(); doReturn(vmNicDao).when(vmInterfaceManager).getVmNicDao(); @@ -111,7 +111,13 @@ OsRepository osRepository = mock(OsRepository.class); when(vmInterfaceManager.getOsRepository()).thenReturn(osRepository); when(osRepository.hasNicHotplugSupport(any(Integer.class), any(Version.class))).thenReturn(true); - vmInterfaceManager.add(iface, NoOpCompensationContext.getInstance(), reserveExistingMac, osId, version); + vmInterfaceManager.add(iface, + NoOpCompensationContext.getInstance(), + reserveExistingMac, + osId, + version, + macPoolManagerStrategy); + if (reserveExistingMac) { verify(macPoolManagerStrategy, times(1)).forceAddMac((iface.getMacAddress())); } else { @@ -154,7 +160,7 @@ when(vmNicDao.getAllForVm(any(Guid.class))).thenReturn(interfaces); - vmInterfaceManager.removeAll(Guid.newGuid()); + vmInterfaceManager.removeAll(Guid.newGuid(), macPoolManagerStrategy); for (VmNic iface : interfaces) { verifyRemoveAllDelegatedCorrectly(iface); -- To view, visit http://gerrit.ovirt.org/36573 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I65aaf5e2c189d2e43e1102b10f621ede0a628aa3 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
