From 4fdb177bb602bd3a81ef62b0b754bc93f8e1dc93 Mon Sep 17 00:00:00 2001 From: Bharat Kumar Date: Wed, 17 Jul 2013 13:05:11 +0530 Subject: [PATCH] Cloudstack-2150 DB table entries of phisical network is not proper.Shows Duplicate entries Cloudstack-2980 Adding a VLAN range that overlaps with two existing ranges results in inconsistent DB entries Signed-off-by: Abhinandan Prateek --- .../src/com/cloud/dc/dao/DataCenterDao.java | 2 +- .../com/cloud/dc/dao/DataCenterDaoImpl.java | 4 +- .../com/cloud/dc/dao/DataCenterVnetDao.java | 4 +- .../cloud/dc/dao/DataCenterVnetDaoImpl.java | 28 ++- .../com/cloud/network/NetworkServiceImpl.java | 160 ++++++------------ .../network/UpdatePhysicalNetworkTest.java | 11 +- 6 files changed, 87 insertions(+), 122 deletions(-) diff --git a/engine/schema/src/com/cloud/dc/dao/DataCenterDao.java b/engine/schema/src/com/cloud/dc/dao/DataCenterDao.java index ed6e6965312..e5e2cdd802c 100755 --- a/engine/schema/src/com/cloud/dc/dao/DataCenterDao.java +++ b/engine/schema/src/com/cloud/dc/dao/DataCenterDao.java @@ -70,7 +70,7 @@ public interface DataCenterDao extends GenericDao { int countZoneVlans(long dcId, boolean onlyCountAllocated); - void addVnet(long dcId, long physicalNetworkId, int start, int end); + void addVnet(long dcId, long physicalNetworkId, List vnets); void deleteVnet(long physicalNetworkId); List listAllocatedVnets(long physicalNetworkId); diff --git a/engine/schema/src/com/cloud/dc/dao/DataCenterDaoImpl.java b/engine/schema/src/com/cloud/dc/dao/DataCenterDaoImpl.java index 503306f6722..47b5522326a 100755 --- a/engine/schema/src/com/cloud/dc/dao/DataCenterDaoImpl.java +++ b/engine/schema/src/com/cloud/dc/dao/DataCenterDaoImpl.java @@ -268,8 +268,8 @@ public class DataCenterDaoImpl extends GenericDaoBase implem } @Override - public void addVnet(long dcId, long physicalNetworkId, int start, int end) { - _vnetAllocDao.add(dcId, physicalNetworkId, start, end); + public void addVnet(long dcId, long physicalNetworkId, List vnets) { + _vnetAllocDao.add(dcId, physicalNetworkId, vnets); } @Override diff --git a/engine/schema/src/com/cloud/dc/dao/DataCenterVnetDao.java b/engine/schema/src/com/cloud/dc/dao/DataCenterVnetDao.java index 778498d8898..e2e6b795d35 100644 --- a/engine/schema/src/com/cloud/dc/dao/DataCenterVnetDao.java +++ b/engine/schema/src/com/cloud/dc/dao/DataCenterVnetDao.java @@ -29,7 +29,7 @@ public interface DataCenterVnetDao extends GenericDao { public int countZoneVlans(long dcId, boolean onlyCountAllocated); public List findVnet(long dcId, long physicalNetworkId, String vnet); - public void add(long dcId, long physicalNetworkId, int start, int end); + public void add(long dcId, long physicalNetworkId, List vnets); public void delete(long physicalNetworkId); @@ -46,4 +46,6 @@ public interface DataCenterVnetDao extends GenericDao { public int countVnetsAllocatedToAccount(long dcId, long accountId); public int countVnetsDedicatedToAccount(long dcId, long accountId); + + List listVnetsByPhysicalNetworkAndDataCenter(long dcId, long physicalNetworkId); } diff --git a/engine/schema/src/com/cloud/dc/dao/DataCenterVnetDaoImpl.java b/engine/schema/src/com/cloud/dc/dao/DataCenterVnetDaoImpl.java index e97f2c62ee3..ced2982cf9d 100755 --- a/engine/schema/src/com/cloud/dc/dao/DataCenterVnetDaoImpl.java +++ b/engine/schema/src/com/cloud/dc/dao/DataCenterVnetDaoImpl.java @@ -64,6 +64,7 @@ public class DataCenterVnetDaoImpl extends GenericDaoBase countVnetsAllocatedToAccount; protected GenericSearchBuilder countVnetsDedicatedToAccount; protected SearchBuilder AccountGuestVlanMapSearch; + protected GenericSearchBuilder ListAllVnetSearch; @Inject protected AccountGuestVlanMapDao _accountGuestVlanMapDao; @@ -112,15 +113,15 @@ public class DataCenterVnetDaoImpl extends GenericDaoBase vnets) { String insertVnet = "INSERT INTO `cloud`.`op_dc_vnet_alloc` (vnet, data_center_id, physical_network_id) VALUES ( ?, ?, ?)"; Transaction txn = Transaction.currentTxn(); try { txn.start(); PreparedStatement stmt = txn.prepareAutoCloseStatement(insertVnet); - for (int i = start; i <= end; i++) { - stmt.setString(1, String.valueOf(i)); + for (int i =0; i <= vnets.size()-1; i++) { + stmt.setString(1, vnets.get(i)); stmt.setLong(2, dcId); stmt.setLong(3, physicalNetworkId); stmt.addBatch(); @@ -128,11 +129,7 @@ public class DataCenterVnetDaoImpl extends GenericDaoBase listVnetsByPhysicalNetworkAndDataCenter(long dcId, long physicalNetworkId){ + SearchCriteria sc = ListAllVnetSearch.create(); + sc.setParameters("dc", dcId ); + sc.setParameters("physicalNetworkId", physicalNetworkId); + return customSearch(sc, null); + } + @Override public boolean configure(String name, Map params) throws ConfigurationException { boolean result = super.configure(name, params); @@ -314,5 +319,12 @@ public class DataCenterVnetDaoImpl extends GenericDaoBase vnets = new ArrayList(); + for (Integer i= vnetStart; i<= vnetEnd; i++ ) { + vnets.add(i.toString()); + } + if (vnetRange != null) { - _dcDao.addVnet(zone.getId(), pNetwork.getId(), vnetStart, vnetEnd); + _dcDao.addVnet(zone.getId(), pNetwork.getId(), vnets); } // add VirtualRouter as the default network service provider @@ -2548,7 +2554,7 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService { @Override @DB @ActionEvent(eventType = EventTypes.EVENT_PHYSICAL_NETWORK_UPDATE, eventDescription = "updating physical network", async = true) - public PhysicalNetwork updatePhysicalNetwork(Long id, String networkSpeed, List tags, String newVnetRangeString, String state, String removeVlan) { + public PhysicalNetwork updatePhysicalNetwork(Long id, String networkSpeed, List tags, String newVnetRange, String state, String removeVlan) { // verify input parameters PhysicalNetworkVO network = _physicalNetworkDao.findById(id); @@ -2565,7 +2571,7 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService { ex.addProxyObject(String.valueOf(network.getDataCenterId()), "dataCenterId"); throw ex; } - if (newVnetRangeString != null) { + if (newVnetRange!= null) { if (zone.getNetworkType() == NetworkType.Basic || (zone.getNetworkType() == NetworkType.Advanced && zone.isSecurityGroupEnabled())) { throw new InvalidParameterValueException("Can't add vnet range to the physical network in the zone that supports " + zone.getNetworkType() + " network, Security Group enabled: " @@ -2575,7 +2581,7 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService { if (removeVlan != null){ List tokens = processVlanRange(network,removeVlan); - boolean result = removeVlanRange(network, tokens.get(0), tokens.get(1)); + removeVlanRange(network, tokens.get(0), tokens.get(1)); } if (tags != null && tags.size() > 1) { @@ -2607,121 +2613,63 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService { boolean AddVnet = true; List> vnetsToAdd = new ArrayList>(); - if (newVnetRangeString != null) { - Integer newStartVnet = 0; - Integer newEndVnet = 0; - List tokens = processVlanRange(network, newVnetRangeString); - newStartVnet = tokens.get(0); - newEndVnet = tokens.get(1); - Integer j=0; - List > existingRanges = network.getVnet(); - if (!existingRanges.isEmpty()) { - for (; j < existingRanges.size(); j++){ - int existingStartVnet = existingRanges.get(j).first(); - int existingEndVnet = existingRanges.get(j).second(); - - // check if vnet is being extended - if (newStartVnet.intValue() >= existingStartVnet & newEndVnet.intValue() <= existingEndVnet) { - throw new InvalidParameterValueException("The vlan range you trying to add already exists."); + List tokens = null; + List add_Vnet = null; + if (newVnetRange != null) { + tokens = processVlanRange(network, newVnetRange); + HashSet vnetsInDb = new HashSet(); + vnetsInDb.addAll(_datacneter_vnet.listVnetsByPhysicalNetworkAndDataCenter(network.getDataCenterId(), id)); + HashSet tempVnets = new HashSet(); + tempVnets.addAll(vnetsInDb); + for (Integer i = tokens.get(0); i <= tokens.get(1); i++) { + tempVnets.add(i.toString()); + } + tempVnets.removeAll(vnetsInDb); + if (tempVnets.isEmpty()) { + throw new InvalidParameterValueException("The vlan range you are trying to add already exists."); + } + vnetsInDb.addAll(tempVnets); + add_Vnet = new ArrayList(); + add_Vnet.addAll(tempVnets); + List sortedList = new ArrayList(vnetsInDb); + Collections.sort(sortedList, new Comparator() { + public int compare(String s1, String s2) { + return Integer.valueOf(s1).compareTo(Integer.valueOf(s2)); } - - if (newStartVnet < existingStartVnet & newEndVnet+1 >= existingStartVnet & newEndVnet <= existingEndVnet) { - vnetsToAdd.add(new Pair(newStartVnet, existingStartVnet - 1)); - existingRanges.get(j).first(newStartVnet); - AddVnet = false; - break; - } - - else if (newStartVnet > existingStartVnet & newStartVnet-1 <= existingEndVnet & newEndVnet >= existingEndVnet) { - vnetsToAdd.add(new Pair(existingEndVnet + 1, newEndVnet)); - existingRanges.get(j).second(newEndVnet); - AddVnet = false; - break; - } - - else if (newStartVnet< existingStartVnet & newEndVnet > existingEndVnet){ - vnetsToAdd.add(new Pair(newStartVnet,existingStartVnet-1)); - vnetsToAdd.add(new Pair(existingEndVnet+1,newEndVnet)); - existingRanges.get(j).first(newStartVnet); - existingRanges.get(j).second(newEndVnet); - break; + }); + //build the vlan string form the allocated vlan list. + String vnetRange = ""; + String startvnet = sortedList.get(0); + String endvnet = ""; + for ( int i =0; i < sortedList.size()-1; i++ ) { + if (Integer.valueOf(sortedList.get(i+1)) - Integer.valueOf(sortedList.get(i)) > 1) { + endvnet = sortedList.get(i); + vnetRange=vnetRange + startvnet+"-"+endvnet+";"; + startvnet = sortedList.get(i+1); } } - + endvnet = sortedList.get(sortedList.size()-1); + vnetRange=vnetRange + startvnet+"-"+endvnet+";"; + vnetRange = vnetRange.substring(0,vnetRange.length()-1); + network.setVnet(vnetRange); } - if (AddVnet){ - vnetsToAdd.add(new Pair(newStartVnet, newEndVnet)); - existingRanges.add(new Pair(newStartVnet,newEndVnet)); - } - - Map vnetMap = new HashMap(existingRanges.size()); - Map IndexMap = new HashMap(existingRanges.size()); - for (int i=0; i< existingRanges.size(); i++){ - vnetMap.put(existingRanges.get(i).first(),existingRanges.get(i).second()); - IndexMap.put(existingRanges.get(i).first(),i); - } - - Integer value; - Integer index; - String vnetString = ""; - for (int i=0; i < existingRanges.size(); i++){ - value = vnetMap.get((existingRanges.get(i).second()+1)); - if (value != null) { - vnetMap.remove((existingRanges.get(i).second()+1)); - vnetMap.remove(existingRanges.get(i).first()); - vnetMap.put(existingRanges.get(i).first(),value); - existingRanges.add(new Pair(existingRanges.get(i).first(),value)); - index = IndexMap.get(existingRanges.get(i).second()+1); - existingRanges.get(index).first(-1); - existingRanges.get(index).second(-1); - existingRanges.get(i).first(-1); - existingRanges.get(i).second(-1); - } - value = vnetMap.get((existingRanges.get(i).second())); - if (value != null && ( (existingRanges.get(i).second()) != (existingRanges.get(i).first()) )) { - vnetMap.remove((existingRanges.get(i).second())); - vnetMap.remove(existingRanges.get(i).first()); - vnetMap.put(existingRanges.get(i).first(),value); - existingRanges.add(new Pair(existingRanges.get(i).first(),value)); - index = IndexMap.get(existingRanges.get(i).second()); - existingRanges.get(index).first(-1); - existingRanges.get(index).second(-1); - existingRanges.get(i).first(-1); - existingRanges.get(i).second(-1); - } - } - - - - if (newVnetRangeString != null) { - for (Pair vnetRange : existingRanges ){ - value=vnetMap.get(vnetRange.first()); - if (value != null){ - vnetString = vnetString+vnetRange.first().toString()+"-"+value.toString()+";"; - } - } - if (vnetString.length() > 0 && vnetString.charAt(vnetString.length()-1)==';') { - vnetString = vnetString.substring(0, vnetString.length()-1); - } - network.setVnet(vnetString); - } - - for (Pair vnetToAdd : vnetsToAdd) { - s_logger.debug("Adding vnet range " + vnetToAdd.first() + "-" + vnetToAdd.second() + " for the physicalNetwork id= " + id + " and zone id=" + network.getDataCenterId() + Transaction txn = Transaction.currentTxn(); + txn.start(); + if (add_Vnet != null) { + s_logger.debug("Adding vnet range " + tokens.get(0).toString() + "-" + tokens.get(1).toString() + " for the physicalNetwork id= " + id + " and zone id=" + network.getDataCenterId() + " as a part of updatePhysicalNetwork call"); - _dcDao.addVnet(network.getDataCenterId(), network.getId(), vnetToAdd.first(), vnetToAdd.second()); - } + _dcDao.addVnet(network.getDataCenterId(), network.getId(), add_Vnet); } - _physicalNetworkDao.update(id, network); + txn.commit(); return network; } - private List processVlanRange(PhysicalNetworkVO network, String removeVlan) { + private List processVlanRange(PhysicalNetworkVO network, String vlan) { Integer StartVnet; Integer EndVnet; - String[] VnetRange = removeVlan.split("-"); + String[] VnetRange = vlan.split("-"); // Init with [min,max] of VLAN. Actually 0x000 and 0xFFF are reserved by IEEE, shoudn't be used. long minVnet = MIN_VLAN_ID; diff --git a/server/test/com/cloud/network/UpdatePhysicalNetworkTest.java b/server/test/com/cloud/network/UpdatePhysicalNetworkTest.java index 25c023e683e..e3fc36a05d2 100644 --- a/server/test/com/cloud/network/UpdatePhysicalNetworkTest.java +++ b/server/test/com/cloud/network/UpdatePhysicalNetworkTest.java @@ -18,6 +18,7 @@ package com.cloud.network; import com.cloud.capacity.CapacityManagerImpl; import com.cloud.dc.DataCenterVO; import com.cloud.dc.dao.DataCenterDao; +import com.cloud.dc.dao.DataCenterVnetDao; import com.cloud.network.NetworkServiceImpl; import com.cloud.network.dao.PhysicalNetworkDao; import com.cloud.network.dao.PhysicalNetworkVO; @@ -40,16 +41,18 @@ import static org.mockito.Mockito.when; public class UpdatePhysicalNetworkTest { private PhysicalNetworkDao _physicalNetworkDao = mock(PhysicalNetworkDao.class); + private DataCenterVnetDao _DatacenterVnetDao = mock(DataCenterVnetDao.class); private DataCenterDao _datacenterDao = mock(DataCenterDao.class); private DataCenterVO datacentervo = mock(DataCenterVO.class); private PhysicalNetworkVO physicalNetworkVO = mock(PhysicalNetworkVO.class); - List> existingRange = new ArrayList>(); + List existingRange = new ArrayList(); ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(String.class); public NetworkServiceImpl setUp() { NetworkServiceImpl networkService = new NetworkServiceImpl(); ((NetworkServiceImpl)networkService)._dcDao= _datacenterDao; networkService._physicalNetworkDao = _physicalNetworkDao; + networkService._datacneter_vnet = _DatacenterVnetDao; return networkService; } @@ -57,15 +60,15 @@ public class UpdatePhysicalNetworkTest { public void updatePhysicalNetworkTest(){ Transaction txn = Transaction.open("updatePhysicalNetworkTest"); NetworkServiceImpl networkService = setUp(); - existingRange.add(new Pair(520, 524)); + existingRange.add("524"); when(_physicalNetworkDao.findById(anyLong())).thenReturn(physicalNetworkVO); when(_datacenterDao.findById(anyLong())).thenReturn(datacentervo); when(_physicalNetworkDao.update(anyLong(), any(physicalNetworkVO.getClass()))).thenReturn(true); - when(physicalNetworkVO.getVnet()).thenReturn(existingRange); + when(_DatacenterVnetDao.listVnetsByPhysicalNetworkAndDataCenter(anyLong(), anyLong())).thenReturn(existingRange); networkService.updatePhysicalNetwork(1l, null, null, "525-530", null, null); txn.close("updatePhysicalNetworkTest"); verify(physicalNetworkVO).setVnet(argumentCaptor.capture()); - assertEquals("520-530", argumentCaptor.getValue()); + assertEquals("524-530", argumentCaptor.getValue()); } }