From ff099fa651e8476220332d7e1cc4b24746802cfe Mon Sep 17 00:00:00 2001 From: Dave Cahill Date: Fri, 22 Feb 2013 17:04:42 +0900 Subject: [PATCH] Adding support for multiple VIF drivers in KVM - Building map of {trafficType, vifDriver} at configure time - Use the relevant VIF driver for the given traffic type when call plug() - Inform all vif drivers when call unplug(), as we no longer know traffic type - Refactor VIF driver choosing code and add unit tests - Basic unit tests, just test default case - Also slight refactor of unit test code, and use jUnit 4 instead of 3, to match rest of codebase Signed-off-by: Hugo Trippaers --- .../resource/LibvirtComputingResource.java | 122 ++++++++-- .../kvm/resource/LibvirtVifDriverTest.java | 226 ++++++++++++++++++ 2 files changed, 325 insertions(+), 23 deletions(-) create mode 100644 plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtVifDriverTest.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 805de408996..5a96c360616 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 @@ -41,6 +41,8 @@ import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.HashSet; import java.util.Properties; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; @@ -276,7 +278,11 @@ ServerResource { private String _mountPoint = "/mnt"; StorageLayer _storage; private KVMStoragePoolManager _storagePoolMgr; - private VifDriver _vifDriver; + + private VifDriver _defaultVifDriver; + private Map _trafficTypeVifDrivers; + protected static final String DEFAULT_OVS_VIF_DRIVER_CLASS_NAME = "com.cloud.hypervisor.kvm.resource.OvsVifDriver"; + protected static final String DEFAULT_BRIDGE_VIF_DRIVER_CLASS_NAME = "com.cloud.hypervisor.kvm.resource.BridgeVifDriver"; private static final class KeyValueInterpreter extends OutputInterpreter { private final Map map = new HashMap(); @@ -775,24 +781,66 @@ ServerResource { params.put("libvirt.host.bridges", bridges); params.put("libvirt.host.pifs", _pifs); - // Load the vif driver - String vifDriverName = (String) params.get("libvirt.vif.driver"); - if (vifDriverName == null) { - if (_bridgeType == BridgeType.OPENVSWITCH) { - s_logger.info("No libvirt.vif.driver specififed. Defaults to OvsVifDriver."); - vifDriverName = "com.cloud.hypervisor.kvm.resource.OvsVifDriver"; - } else { - s_logger.info("No libvirt.vif.driver specififed. Defaults to BridgeVifDriver."); - vifDriverName = "com.cloud.hypervisor.kvm.resource.BridgeVifDriver"; - } - } - params.put("libvirt.computing.resource", this); + configureVifDrivers(params); + + return true; + } + + protected void configureVifDrivers(Map params) + throws ConfigurationException { + final String LIBVIRT_VIF_DRIVER = "libvirt.vif.driver"; + + _trafficTypeVifDrivers = new HashMap(); + + // Load the default vif driver + String defaultVifDriverName = (String) params.get(LIBVIRT_VIF_DRIVER); + if (defaultVifDriverName == null) { + if (_bridgeType == BridgeType.OPENVSWITCH) { + s_logger.info("No libvirt.vif.driver specified. Defaults to OvsVifDriver."); + defaultVifDriverName = DEFAULT_OVS_VIF_DRIVER_CLASS_NAME; + } else { + s_logger.info("No libvirt.vif.driver specified. Defaults to BridgeVifDriver."); + defaultVifDriverName = DEFAULT_BRIDGE_VIF_DRIVER_CLASS_NAME; + } + } + _defaultVifDriver = getVifDriverClass(defaultVifDriverName, params); + + // Load any per-traffic-type vif drivers + for (Map.Entry entry : params.entrySet()) + { + String k = entry.getKey(); + String vifDriverPrefix = LIBVIRT_VIF_DRIVER + "."; + + if(k.startsWith(vifDriverPrefix)){ + // Get trafficType + String trafficTypeSuffix = k.substring(vifDriverPrefix.length()); + + // Does this suffix match a real traffic type? + TrafficType trafficType = TrafficType.getTrafficType(trafficTypeSuffix); + if(!trafficType.equals(TrafficType.None)){ + // Get vif driver class name + String vifDriverClassName = (String) entry.getValue(); + // if value is null, ignore + if(vifDriverClassName != null){ + // add traffic type to vif driver mapping to Map + _trafficTypeVifDrivers.put(trafficType, + getVifDriverClass(vifDriverClassName, params)); + } + } + } + } + } + + protected VifDriver getVifDriverClass(String vifDriverClassName, Map params) + throws ConfigurationException { + VifDriver vifDriver; + try { - Class clazz = Class.forName(vifDriverName); - _vifDriver = (VifDriver) clazz.newInstance(); - _vifDriver.configure(params); + Class clazz = Class.forName(vifDriverClassName); + vifDriver = (VifDriver) clazz.newInstance(); + vifDriver.configure(params); } catch (ClassNotFoundException e) { throw new ConfigurationException("Unable to find class for libvirt.vif.driver " + e); } catch (InstantiationException e) { @@ -800,8 +848,28 @@ ServerResource { } catch (Exception e) { throw new ConfigurationException("Failed to initialize libvirt.vif.driver " + e); } + return vifDriver; + } - return true; + protected VifDriver getVifDriver(TrafficType trafficType){ + VifDriver vifDriver = _trafficTypeVifDrivers.get(trafficType); + + if(vifDriver == null){ + vifDriver = _defaultVifDriver; + } + + return vifDriver; + } + + protected List getAllVifDrivers(){ + Set vifDrivers = new HashSet(); + + vifDrivers.add(_defaultVifDriver); + vifDrivers.addAll(_trafficTypeVifDrivers.values()); + + ArrayList vifDriverList = new ArrayList(vifDrivers); + + return vifDriverList; } private void getPifs() { @@ -1443,7 +1511,7 @@ ServerResource { } Domain vm = getDomain(conn, vmName); - vm.attachDevice(_vifDriver.plug(nicTO, "Other PV (32-bit)").toString()); + vm.attachDevice(getVifDriver(nicTO.getType()).plug(nicTO, "Other PV (32-bit)").toString()); } private PlugNicAnswer execute(PlugNicCommand cmd) { @@ -1462,7 +1530,7 @@ ServerResource { } nicnum++; } - vm.attachDevice(_vifDriver.plug(nic, "Other PV (32-bit)").toString()); + vm.attachDevice(getVifDriver(nic.getType()).plug(nic, "Other PV (32-bit)").toString()); return new PlugNicAnswer(cmd, true, "success"); } catch (Exception e) { String msg = " Plug Nic failed due to " + e.toString(); @@ -2560,7 +2628,11 @@ ServerResource { } else { destroy_network_rules_for_vm(conn, vmName); for (InterfaceDef iface : ifaces) { - _vifDriver.unplug(iface); + // 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(iface); + } } cleanupVM(conn, vmName, getVnetId(VirtualMachineName.getVnet(vmName))); @@ -2580,7 +2652,7 @@ ServerResource { try { Connect conn = LibvirtConnection.getConnection(); for (NicTO nic : nics) { - _vifDriver.plug(nic, null); + getVifDriver(nic.getType()).plug(nic, null); } /* setup disks, e.g for iso */ @@ -2794,7 +2866,11 @@ ServerResource { } } for (InterfaceDef iface: ifaces) { - _vifDriver.unplug(iface); + // 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(iface); + } } } @@ -3231,7 +3307,7 @@ ServerResource { private void createVif(LibvirtVMDef vm, NicTO nic) throws InternalErrorException, LibvirtException { vm.getDevices().addDevice( - _vifDriver.plug(nic, vm.getGuestOSType()).toString()); + getVifDriver(nic.getType()).plug(nic, vm.getGuestOSType()).toString()); } protected CheckSshAnswer execute(CheckSshCommand cmd) { diff --git a/plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtVifDriverTest.java b/plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtVifDriverTest.java new file mode 100644 index 00000000000..7d47e6e81de --- /dev/null +++ b/plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtVifDriverTest.java @@ -0,0 +1,226 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package com.cloud.hypervisor.kvm.resource; + +import com.cloud.network.Networks.TrafficType; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.BridgeType; + +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +import java.util.HashMap; +import java.util.Map; +import javax.naming.ConfigurationException; + + +import static org.mockito.Mockito.*; + +public class LibvirtVifDriverTest { + private LibvirtComputingResource res; + + private Map assertions; + + final String LIBVIRT_VIF_DRIVER = "libvirt.vif.driver"; + final String FAKE_VIF_DRIVER_CLASS_NAME = "com.cloud.hypervisor.kvm.resource.FakeVifDriver"; + final String NONEXISTENT_VIF_DRIVER_CLASS_NAME = "com.cloud.hypervisor.kvm.resource.NonExistentVifDriver"; + + private VifDriver fakeVifDriver, bridgeVifDriver, ovsVifDriver; + + @Before + public void setUp() { + // Use a spy because we only want to override getVifDriverClass + LibvirtComputingResource resReal = new LibvirtComputingResource(); + res = spy(resReal); + + try{ + bridgeVifDriver = + (VifDriver) Class.forName(LibvirtComputingResource.DEFAULT_BRIDGE_VIF_DRIVER_CLASS_NAME).newInstance(); + ovsVifDriver = + (VifDriver) Class.forName(LibvirtComputingResource.DEFAULT_OVS_VIF_DRIVER_CLASS_NAME).newInstance(); + + // Instantiating bridge vif driver again as the fake vif driver + // is good enough, as this is a separate instance + fakeVifDriver = + (VifDriver) Class.forName(LibvirtComputingResource.DEFAULT_BRIDGE_VIF_DRIVER_CLASS_NAME).newInstance(); + + doReturn(bridgeVifDriver).when(res) + .getVifDriverClass(eq(LibvirtComputingResource.DEFAULT_BRIDGE_VIF_DRIVER_CLASS_NAME), anyMap()); + doReturn(ovsVifDriver).when(res) + .getVifDriverClass(eq(LibvirtComputingResource.DEFAULT_OVS_VIF_DRIVER_CLASS_NAME), anyMap()); + doReturn(fakeVifDriver).when(res) + .getVifDriverClass(eq(FAKE_VIF_DRIVER_CLASS_NAME), anyMap()); + + } catch (final ConfigurationException ex){ + fail("Unexpected ConfigurationException while configuring VIF drivers: " + ex.getMessage()); + } catch (final Exception ex){ + fail("Unexpected Exception while configuring VIF drivers"); + } + + assertions = new HashMap(); + } + + + // Helper function + // Configure LibvirtComputingResource using params + private void configure (Map params) + throws ConfigurationException{ + res.configureVifDrivers(params); + } + + // Helper function + private void checkAssertions(){ + // Check the defined assertions + for (Map.Entry assertion : assertions.entrySet()){ + assertEquals(res.getVifDriver(assertion.getKey()), + assertion.getValue()); + } + } + + // Helper when all answers should be the same + private void checkAllSame(VifDriver vifDriver) + throws ConfigurationException { + + for(TrafficType trafficType : TrafficType.values()){ + assertions.put(trafficType, vifDriver); + } + + checkAssertions(); + } + + @Test + public void testDefaults() + throws ConfigurationException { + // If no special vif driver settings, all traffic types should + // map to the default vif driver for the bridge type + Map params = new HashMap(); + + res._bridgeType = BridgeType.NATIVE; + configure(params); + checkAllSame(bridgeVifDriver); + + res._bridgeType = BridgeType.OPENVSWITCH; + configure(params); + checkAllSame(ovsVifDriver); + } + + @Test + public void testDefaultsWhenExplicitlySet() + throws ConfigurationException { + + Map params = new HashMap(); + + // Switch res' bridge type for test purposes + params.put(LIBVIRT_VIF_DRIVER, LibvirtComputingResource.DEFAULT_BRIDGE_VIF_DRIVER_CLASS_NAME); + res._bridgeType = BridgeType.NATIVE; + configure(params); + checkAllSame(bridgeVifDriver); + + params.clear(); + params.put(LIBVIRT_VIF_DRIVER, LibvirtComputingResource.DEFAULT_OVS_VIF_DRIVER_CLASS_NAME); + res._bridgeType = BridgeType.OPENVSWITCH; + configure(params); + checkAllSame(ovsVifDriver); + } + + @Test + public void testWhenExplicitlySetDifferentDefault() + throws ConfigurationException { + + // Tests when explicitly set vif driver to OVS when using regular bridges and vice versa + Map params = new HashMap(); + + // Switch res' bridge type for test purposes + params.put(LIBVIRT_VIF_DRIVER, LibvirtComputingResource.DEFAULT_OVS_VIF_DRIVER_CLASS_NAME); + res._bridgeType = BridgeType.NATIVE; + configure(params); + checkAllSame(ovsVifDriver); + + params.clear(); + params.put(LIBVIRT_VIF_DRIVER, LibvirtComputingResource.DEFAULT_BRIDGE_VIF_DRIVER_CLASS_NAME); + res._bridgeType = BridgeType.OPENVSWITCH; + configure(params); + checkAllSame(bridgeVifDriver); + } + + @Test + public void testOverrideSomeTrafficTypes() + throws ConfigurationException { + + Map params = new HashMap(); + params.put(LIBVIRT_VIF_DRIVER + "." + "Public", FAKE_VIF_DRIVER_CLASS_NAME); + params.put(LIBVIRT_VIF_DRIVER + "." + "Guest", + LibvirtComputingResource.DEFAULT_OVS_VIF_DRIVER_CLASS_NAME); + res._bridgeType = BridgeType.NATIVE; + configure(params); + + // Initially, set all traffic types to use default + for(TrafficType trafficType : TrafficType.values()){ + assertions.put(trafficType, bridgeVifDriver); + } + + assertions.put(TrafficType.Public, fakeVifDriver); + assertions.put(TrafficType.Guest, ovsVifDriver); + + checkAssertions(); + } + + @Test + public void testBadTrafficType() + throws ConfigurationException { + Map params = new HashMap(); + params.put(LIBVIRT_VIF_DRIVER + "." + "NonExistentTrafficType", FAKE_VIF_DRIVER_CLASS_NAME); + res._bridgeType = BridgeType.NATIVE; + configure(params); + + // Set all traffic types to use default, because bad traffic type should be ignored + for(TrafficType trafficType : TrafficType.values()){ + assertions.put(trafficType, bridgeVifDriver); + } + + checkAssertions(); + } + + @Test + public void testEmptyTrafficType() + throws ConfigurationException { + Map params = new HashMap(); + params.put(LIBVIRT_VIF_DRIVER + ".", FAKE_VIF_DRIVER_CLASS_NAME); + res._bridgeType = BridgeType.NATIVE; + configure(params); + + // Set all traffic types to use default, because bad traffic type should be ignored + for(TrafficType trafficType : TrafficType.values()){ + assertions.put(trafficType, bridgeVifDriver); + } + + checkAssertions(); + } + + @Test(expected=ConfigurationException.class) + public void testBadVifDriverClassName() + throws ConfigurationException { + Map params = new HashMap(); + params.put(LIBVIRT_VIF_DRIVER + "." + "Public", NONEXISTENT_VIF_DRIVER_CLASS_NAME); + res._bridgeType = BridgeType.NATIVE; + configure(params); + } +}