Updated Branches: refs/heads/4.2 f1636203f -> 8065a5570 refs/heads/master b81f82414 -> b13485493
CLOUDSTACK-3431: KVM: cloudstack-plugin-hypervisor-kvm with BridgeVifDriver doesn't cleanup vNet due to multiple reasons - Move vnetBridge clean up function from LibvirtComputingResource to BridgeVifDriver -- since only BridgeVifDriver have to handle this event - LibvirtComputingResource now properly call VifDriver.unplug() when it receives UnPlugCommand - Remove not working and no longer used method getVnet(String) from VirtualMachineName - Remove not working and no longer used method getVnet() from StopCommand - Remove unused constructer StopCommand(VirtualMachine, String, boolean) from StopCommand - Remove unused member vnet from StopCommand - Remove unused member _modifyVlanPath from OvsVifDriver Tested with 2 KVM hosts and confirmed it correctly manipulate vnetBridge with start, stop, migrate, plug, and unplug event Signed-off-by: Hugo Trippaers <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/8065a557 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/8065a557 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/8065a557 Branch: refs/heads/4.2 Commit: 8065a5570d44467b616841a821a67a2e13ceb5ec Parents: f163620 Author: Toshiaki Hatano <[email protected]> Authored: Fri Jul 12 03:49:35 2013 +0000 Committer: Hugo Trippaers <[email protected]> Committed: Mon Jul 15 09:11:43 2013 +0200 ---------------------------------------------------------------------- api/src/com/cloud/vm/VirtualMachineName.java | 4 -- core/src/com/cloud/agent/api/StopCommand.java | 11 --- .../kvm/resource/BridgeVifDriver.java | 74 +++++++++++++++++--- .../kvm/resource/LibvirtComputingResource.java | 68 ++---------------- .../hypervisor/kvm/resource/OvsVifDriver.java | 6 -- 5 files changed, 70 insertions(+), 93 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/8065a557/api/src/com/cloud/vm/VirtualMachineName.java ---------------------------------------------------------------------- diff --git a/api/src/com/cloud/vm/VirtualMachineName.java b/api/src/com/cloud/vm/VirtualMachineName.java index 81838b9..1279fd6 100755 --- a/api/src/com/cloud/vm/VirtualMachineName.java +++ b/api/src/com/cloud/vm/VirtualMachineName.java @@ -93,10 +93,6 @@ public class VirtualMachineName { return Long.parseLong(vmName.substring(begin + 1, end)); } - public static String getVnet(String vmName) { - return vmName.substring(vmName.lastIndexOf(SEPARATOR) + SEPARATOR.length()); - } - public static String getRouterName(long routerId, String instance) { StringBuilder builder = new StringBuilder("r"); builder.append(SEPARATOR).append(routerId).append(SEPARATOR).append(instance); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/8065a557/core/src/com/cloud/agent/api/StopCommand.java ---------------------------------------------------------------------- diff --git a/core/src/com/cloud/agent/api/StopCommand.java b/core/src/com/cloud/agent/api/StopCommand.java index a3ee3c9..65a6563 100755 --- a/core/src/com/cloud/agent/api/StopCommand.java +++ b/core/src/com/cloud/agent/api/StopCommand.java @@ -19,7 +19,6 @@ package com.cloud.agent.api; import com.cloud.vm.VirtualMachine; public class StopCommand extends RebootCommand { - String vnet; private boolean isProxy=false; private String urlPort=null; private String publicConsoleProxyIpAddress=null; @@ -35,12 +34,6 @@ public class StopCommand extends RebootCommand { this.publicConsoleProxyIpAddress = publicConsoleProxyIpAddress; this.executeInSequence = executeInSequence; } - - public StopCommand(VirtualMachine vm, String vnet, boolean executeInSequence) { - super(vm); - this.vnet = vnet; - this.executeInSequence = executeInSequence; - } public StopCommand(VirtualMachine vm, boolean executeInSequence) { super(vm); @@ -52,10 +45,6 @@ public class StopCommand extends RebootCommand { this.executeInSequence = executeInSequence; } - public String getVnet() { - return vnet; - } - @Override public boolean executeInSequence() { return executeInSequence; http://git-wip-us.apache.org/repos/asf/cloudstack/blob/8065a557/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java ---------------------------------------------------------------------- diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java index 0db83cc..195cf40 100644 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java @@ -33,6 +33,8 @@ import org.libvirt.LibvirtException; import javax.naming.ConfigurationException; import java.net.URI; import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.io.File; public class BridgeVifDriver extends VifDriverBase { @@ -40,6 +42,8 @@ public class BridgeVifDriver extends VifDriverBase { private static final Logger s_logger = Logger .getLogger(BridgeVifDriver.class); private int _timeout; + + private static final Object _vnetBridgeMonitor = new Object(); private String _modifyVlanPath; @Override @@ -136,7 +140,7 @@ public class BridgeVifDriver extends VifDriverBase { @Override public void unplug(LibvirtVMDef.InterfaceDef iface) { - // Nothing needed as libvirt cleans up tap interface from bridge. + deleteVnetBr(iface.getBrName()); } private String setVnetBrName(String pifName, String vnetId) { @@ -161,16 +165,64 @@ public class BridgeVifDriver extends VifDriverBase { private void createVnet(String vnetId, String pif, String brName) throws InternalErrorException { - final Script command = new Script(_modifyVlanPath, _timeout, s_logger); - command.add("-v", vnetId); - command.add("-p", pif); - command.add("-b", brName); - command.add("-o", "add"); - - final String result = command.execute(); - if (result != null) { - throw new InternalErrorException("Failed to create vnet " + vnetId - + ": " + result); + synchronized (_vnetBridgeMonitor) { + final Script command = new Script(_modifyVlanPath, _timeout, s_logger); + command.add("-v", vnetId); + command.add("-p", pif); + command.add("-b", brName); + command.add("-o", "add"); + + final String result = command.execute(); + if (result != null) { + throw new InternalErrorException("Failed to create vnet " + vnetId + + ": " + result); + } + } + } + + private void deleteVnetBr(String brName){ + synchronized (_vnetBridgeMonitor) { + String cmdout = Script.runSimpleBashScript("ls /sys/class/net/" + brName + "/brif | grep vnet"); + if (cmdout != null && cmdout.contains("vnet")) { + // Active VM remains on that bridge + return; + } + + Pattern oldStyleBrNameRegex = Pattern.compile("^cloudVirBr(\\d+)$"); + Pattern brNameRegex = Pattern.compile("^br(\\S+)-(\\d+)$"); + Matcher oldStyleBrNameMatcher = oldStyleBrNameRegex.matcher(brName); + Matcher brNameMatcher = brNameRegex.matcher(brName); + + String pName = null; + String vNetId = null; + if (oldStyleBrNameMatcher.find()) { + // Actually modifyvlan.sh doesn't require pif name when deleting its bridge so far. + pName = "undefined"; + vNetId = oldStyleBrNameMatcher.group(1); + } else if (brNameMatcher.find()) { + if(brNameMatcher.group(1) != null || !brNameMatcher.group(1).isEmpty()) { + pName = brNameMatcher.group(1); + } else { + pName = "undefined"; + } + vNetId = brNameMatcher.group(2); + } + + if (vNetId == null || vNetId.isEmpty()) { + s_logger.debug("unable to get a vNet ID from name "+ brName); + return; + } + + final Script command = new Script(_modifyVlanPath, _timeout, s_logger); + command.add("-o", "delete"); + command.add("-v", vNetId); + command.add("-p", pName); + command.add("-b", brName); + + final String result = command.execute(); + if (result != null) { + s_logger.debug("Delete bridge " + brName + " failed: " + result); + } } } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/8065a557/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---------------------------------------------------------------------- diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 24f9ee0..a2e4044 100755 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -1065,10 +1065,6 @@ ServerResource { } } - private String getVnetId(String vnetId) { - return vnetId; - } - private void passCmdLine(String vmName, String cmdLine) throws InternalErrorException { final Script command = new Script(_patchViaSocketPath, _timeout, s_logger); @@ -1694,6 +1690,11 @@ ServerResource { for (InterfaceDef pluggedNic : pluggedNics) { if (pluggedNic.getMacAddress().equalsIgnoreCase(nic.getMac())) { vm.detachDevice(pluggedNic.toString()); + // We don't know which "traffic type" is associated with + // each interface at this point, so inform all vif drivers + for(VifDriver vifDriver : getAllVifDrivers()){ + vifDriver.unplug(pluggedNic); + } return new UnPlugNicAnswer(cmd, true, "success"); } } @@ -2726,8 +2727,6 @@ ServerResource { vifDriver.unplug(iface); } } - cleanupVM(conn, vmName, - getVnetId(VirtualMachineName.getVnet(vmName))); } return new MigrateAnswer(cmd, result == null, result, null); @@ -3043,11 +3042,6 @@ ServerResource { } } - final String result2 = cleanupVnet(conn, cmd.getVnet()); - - if (result != null && result2 != null) { - result = result2 + result; - } state = State.Stopped; return new StopAnswer(cmd, result, 0, true); } catch (LibvirtException e) { @@ -4123,16 +4117,6 @@ ServerResource { return info; } - protected void cleanupVM(Connect conn, final String vmName, - final String vnet) { - s_logger.debug("Trying to cleanup the vnet: " + vnet); - if (vnet != null) { - cleanupVnet(conn, vnet); - } - - _vmStats.remove(vmName); - } - protected String rebootVM(Connect conn, String vmName) { Domain dm = null; String msg = null; @@ -4274,29 +4258,6 @@ ServerResource { return null; } - public synchronized String cleanupVnet(Connect conn, final String vnetId) { - // VNC proxy VMs do not have vnet - if (vnetId == null || vnetId.isEmpty() - || isDirectAttachedNetwork(vnetId)) { - return null; - } - - final List<String> names = getAllVmNames(conn); - - if (!names.isEmpty()) { - for (final String name : names) { - if (VirtualMachineName.getVnet(name).equals(vnetId)) { - return null; // Can't remove the vnet yet. - } - } - } - - final Script command = new Script(_modifyVlanPath, _timeout, s_logger); - command.add("-o", "delete"); - command.add("-v", vnetId); - return command.execute(); - } - protected Integer getVncPort(Connect conn, String vmName) throws LibvirtException { LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser(); @@ -4410,26 +4371,11 @@ ServerResource { } } - private String getVnetIdFromBrName(String vnetBrName) { - if (vnetBrName.contains("cloudVirBr")) { - return vnetBrName.replaceAll("cloudVirBr", ""); - } else { - Pattern r = Pattern.compile("-(\\d+)$"); - Matcher m = r.matcher(vnetBrName); - if(m.group(1) != null || !m.group(1).isEmpty()) { - return m.group(1); - } else { - s_logger.debug("unable to get a vlan ID from name " + vnetBrName); - return ""; - } - } - } - private void cleanupVMNetworks(Connect conn, List<InterfaceDef> nics) { if (nics != null) { for (InterfaceDef nic : nics) { - if (nic.getHostNetType() == hostNicType.VNET) { - cleanupVnet(conn, getVnetIdFromBrName(nic.getBrName())); + for(VifDriver vifDriver : getAllVifDrivers()){ + vifDriver.unplug(nic); } } } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/8065a557/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/OvsVifDriver.java ---------------------------------------------------------------------- diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/OvsVifDriver.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/OvsVifDriver.java index 951badd..7038d7e 100644 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/OvsVifDriver.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/OvsVifDriver.java @@ -38,7 +38,6 @@ import com.cloud.utils.script.Script; public class OvsVifDriver extends VifDriverBase { private static final Logger s_logger = Logger.getLogger(OvsVifDriver.class); private int _timeout; - private String _modifyVlanPath; @Override public void configure(Map<String, Object> params) throws ConfigurationException { @@ -52,11 +51,6 @@ public class OvsVifDriver extends VifDriverBase { String value = (String) params.get("scripts.timeout"); _timeout = NumbersUtil.parseInt(value, 30 * 60) * 1000; - _modifyVlanPath = Script.findScript(networkScriptsDir, "modifyvlan.sh"); - if (_modifyVlanPath == null) { - throw new ConfigurationException("Unable to find modifyvlan.sh"); - } - createControlNetwork(_bridges.get("linklocal")); }
