Mike Kolesnik has posted comments on this change.
Change subject: core: handle nic ordering when adding or importing a VM
......................................................................
Patch Set 7:
(6 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 1059:
Line 1060: vmInterfaceManager.sortVmNics(nics,
getVm().getStaticData().getManagedDeviceMap());
Line 1061:
Line 1062: // If we import it as a new entity, then we allocate all MAC
addresses in advance
Line 1063: List<String> macAddresses = ListUtils.EMPTY_LIST;
Why assign empty list if you only use this in import as new?
You can have it all under one block (which is more readable as it's iin one
place and not spread across the method):
if (getParameters().isImportAsNewEntity()) {
List<String> macAddresses =
MacPoolManager.getInstance().allocateMacAddresses(nics.size());
for (int i = 0; i < nics.size(); ++i) {
iface.setMacAddress(macAddresses.get(i));
}
}
Line 1064: if (getParameters().isImportAsNewEntity()) {
Line 1065: macAddresses =
MacPoolManager.getInstance().allocateMacAddresses(nics.size());
Line 1066: }
Line 1067:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManager.java
Line 332: private void decrementMacInMap(Map<String, Integer> macMap,
String mac) {
Line 333: macMap.put(mac, macMap.get(mac) - 1);
Line 334: }
Line 335:
Line 336: public List<String> allocateMacAddresses(int numberOfAddresses) {
Please add javadoc describing that the mac addresses are sorted and are in
ascending order
Line 337: List<String> macAddresses = new
ArrayList<String>(numberOfAddresses);
Line 338:
Line 339: for (int i = 0; i < numberOfAddresses; ++i) {
Line 340:
macAddresses.add(MacPoolManager.getInstance().allocateNewMac());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java
Line 58: * @return <code>true</code> if the MAC wasn't used,
<code>false</code> if it was.
Line 59: */
Line 60: public void add(final VmNic iface,
Line 61: CompensationContext compensationContext,
Line 62: boolean allocateExistingMac,
Perhaps a better name would be "reserveExistingMac" which better describes what
we actually doing.
Also this can be better documented in the javadoc, i.e. "Used to denote if we
want to reserve the NIC's MAC address in the {@link MacPoolManager}"
Line 63: int osId,
Line 64: Version clusterCompatibilityVersion) {
Line 65:
Line 66: if (allocateExistingMac) {
Line 78: compensationContext.snapshotNewEntity(iface);
Line 79: compensationContext.snapshotNewEntity(iface.getStatistics());
Line 80: }
Line 81:
Line 82: public void add(final VmNic iface,
Is this still used?
Line 83: CompensationContext compensationContext,
Line 84: String macAddress,
Line 85: int osId,
Line 86: Version clusterCompatibilityVersion) {
Line 192: }
Line 193: return false;
Line 194: }
Line 195:
Line 196: public void sortVmNics(List<? extends VmNic> nics, final
Map<Guid, VmDevice> vmInterfaceDevices) {
Please add javadoc documenting the output from the method and why it's like that
Line 197: Collections.sort(nics, new Comparator<VmNic>() {
Line 198: @Override
Line 199: public int compare(VmNic nic1, VmNic nic2) {
Line 200: if (vmInterfaceDevices != null) {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/VmInterfaceManagerTest.java
Line 108: Version version) {
Line 109: OsRepository osRepository = mock(OsRepository.class);
Line 110:
when(vmInterfaceManager.getOsRepository()).thenReturn(osRepository);
Line 111: when(osRepository.hasNicHotplugSupport(any(Integer.class),
any(Version.class))).thenReturn(true);
Line 112: vmInterfaceManager.add(iface,
NoOpCompensationContext.getInstance(), true, osId, version);
Please add a test that check that when sending false, no interaction with
MacPoolManager occurs
Line 113: verify(macPoolManager,
times(1)).forceAddMac((iface.getMacAddress()));
Line 114: verifyAddDelegatedCorrectly(iface, addMacVerification);
Line 115: }
Line 116:
--
To view, visit http://gerrit.ovirt.org/22411
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I46b4ffc4ecaab824b90bf8b8ecdfe4ebad3074bf
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Livnat Peer <lp...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
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
