From f60f3cec34d0f2471d08cf07b936f7511b5e7eec Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Wed, 8 Aug 2018 12:05:42 +0530 Subject: [PATCH] router: Fixes #2789 fix proper mark based packet routing across interfaces (#2791) Previously, the ethernet device index was used as rt_table index and packet marking id/integer. With eth0 that is sometimes used as link-local interface, the rt_table index `0` would fail as `0` is already defined as a catchall (unspecified). The fwmarking on packets on eth0 with 0x0 would also fail. This fixes the routing issues, by adding 100 to the ethernet device index so the value is a non-zero, for example then the relationship between rt_table index and ethernet would be like: 100 -> Table_eth0 -> eth0 -> fwmark 100 or 0x64 101 -> Table_eth1 -> eth1 -> fwmark 101 or 0x65 102 -> Table_eth2 -> eth2 -> fwmark 102 or 0x66 This would maintain the legacy design of routing based on packet mark and appropriate routing table rules per table/ids. This also fixes a minor NPE issue around listing of snapshots. This also backports fixes to smoketests from master. Signed-off-by: Rohit Yadav --- .../src/com/cloud/api/ApiResponseHelper.java | 2 +- systemvm/debian/opt/cloud/bin/cloud-nic.sh | 2 +- .../debian/opt/cloud/bin/cs/CsNetfilter.py | 3 +- systemvm/debian/opt/cloud/bin/cs/CsRoute.py | 2 +- systemvm/debian/opt/cloud/bin/cs/CsRule.py | 2 +- systemvm/debian/opt/cloud/bin/ipassoc.sh | 4 +- .../debian/opt/cloud/bin/update_config.py | 21 ++--- .../smoke/test_deploy_virtio_scsi_vm.py | 78 +++++++++---------- test/integration/smoke/test_vm_life_cycle.py | 2 +- 9 files changed, 56 insertions(+), 60 deletions(-) diff --git a/server/src/com/cloud/api/ApiResponseHelper.java b/server/src/com/cloud/api/ApiResponseHelper.java index 85ffaf59c03..c10e2598386 100644 --- a/server/src/com/cloud/api/ApiResponseHelper.java +++ b/server/src/com/cloud/api/ApiResponseHelper.java @@ -501,7 +501,7 @@ public class ApiResponseHelper implements ResponseGenerator { snapshotResponse.setZoneId(zone.getUuid()); } - if (volume.getVolumeType() == Volume.Type.ROOT) { + if (volume.getVolumeType() == Volume.Type.ROOT && volume.getInstanceId() != null) { //TODO combine lines and 489 into a join in the volume dao VMInstanceVO instance = ApiDBUtils.findVMInstanceById(volume.getInstanceId()); if (instance != null) { diff --git a/systemvm/debian/opt/cloud/bin/cloud-nic.sh b/systemvm/debian/opt/cloud/bin/cloud-nic.sh index ad7e6e5f9f7..7d8bda83d29 100755 --- a/systemvm/debian/opt/cloud/bin/cloud-nic.sh +++ b/systemvm/debian/opt/cloud/bin/cloud-nic.sh @@ -69,7 +69,7 @@ unplug_nic() { action=$1 dev=$2 -tableNo=${dev:3} +tableNo=$((100+${dev:3})) tableName="Table_$dev" if [ $action == 'add' ] diff --git a/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py b/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py index a9ac0ad5262..01dfa7cac39 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py @@ -189,7 +189,8 @@ class CsNetfilters(object): def add_chain(self, rule): """ Add the given chain if it is not already present """ if not self.has_chain(rule.get_table(), rule.get_chain()): - CsHelper.execute("iptables -t %s -N %s" % (rule.get_table(), rule.get_chain())) + if rule.get_chain(): + CsHelper.execute("iptables -t %s -N %s" % (rule.get_table(), rule.get_chain())) self.chain.add(rule.get_table(), rule.get_chain()) def del_standard(self): diff --git a/systemvm/debian/opt/cloud/bin/cs/CsRoute.py b/systemvm/debian/opt/cloud/bin/cs/CsRoute.py index 74544d98833..47d3d2a91af 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsRoute.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsRoute.py @@ -30,7 +30,7 @@ class CsRoute: return self.table_prefix + name def add_table(self, devicename): - tablenumber = devicename[3:] + tablenumber = 100 + int(devicename[3:]) tablename = self.get_tablename(devicename) str = "%s %s" % (tablenumber, tablename) filename = "/etc/iproute2/rt_tables" diff --git a/systemvm/debian/opt/cloud/bin/cs/CsRule.py b/systemvm/debian/opt/cloud/bin/cs/CsRule.py index 85953fe6561..f1caa298904 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsRule.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsRule.py @@ -27,7 +27,7 @@ class CsRule: def __init__(self, dev): self.dev = dev - self.tableNo = int(dev[3:]) + self.tableNo = 100 + int(dev[3:]) self.table = "Table_%s" % (dev) def addRule(self, rule): diff --git a/systemvm/debian/opt/cloud/bin/ipassoc.sh b/systemvm/debian/opt/cloud/bin/ipassoc.sh index e2f95a3af11..9bcb13279d7 100755 --- a/systemvm/debian/opt/cloud/bin/ipassoc.sh +++ b/systemvm/debian/opt/cloud/bin/ipassoc.sh @@ -103,7 +103,7 @@ remove_routing() { logger -t cloud "$(basename $0):Remove routing $pubIp on interface $ethDev" local ipNoMask=$(echo $pubIp | awk -F'/' '{print $1}') local mask=$(echo $pubIp | awk -F'/' '{print $2}') - local tableNo=$(echo $ethDev | awk -F'eth' '{print $2}') + local tableNo=$((100+$(echo $ethDev | awk -F'eth' '{print $2}'))) local tableName="Table_$ethDev" local remainip=`ip addr show $ethDev | grep "inet "` @@ -149,7 +149,7 @@ add_routing() { local tableName="Table_$ethDev" local tablePresent=$(grep $tableName /etc/iproute2/rt_tables) - local tableNo=$(echo $ethDev | awk -F'eth' '{print $2}') + local tableNo=$((100+$(echo $ethDev | awk -F'eth' '{print $2}'))) if [ "$tablePresent" == "" ] then if [ "$tableNo" == "" ] diff --git a/systemvm/debian/opt/cloud/bin/update_config.py b/systemvm/debian/opt/cloud/bin/update_config.py index f79e74c828f..02161b662e5 100755 --- a/systemvm/debian/opt/cloud/bin/update_config.py +++ b/systemvm/debian/opt/cloud/bin/update_config.py @@ -30,7 +30,7 @@ logging.basicConfig(filename='/var/log/cloud.log', level=logging.INFO, format='% # first commandline argument should be the file to process if (len(sys.argv) != 2): - print "[ERROR]: Invalid usage" + logging.error("Invalid usage, args passed: %s" % sys.argv) sys.exit(1) # FIXME we should get this location from a configuration class @@ -47,7 +47,7 @@ def finish_config(): def process_file(): - print "[INFO] Processing JSON file %s" % sys.argv[1] + logging.info("Processing JSON file %s" % sys.argv[1]) qf = QueueFile() qf.setFile(sys.argv[1]) qf.load(None) @@ -70,7 +70,7 @@ def is_guestnet_configured(guestnet_dict, keys): ''' It seems all the interfaces have been removed. Let's allow a new configuration to come in. ''' - print "[WARN] update_config.py :: Reconfiguring guest network..." + logging.warn("update_config.py :: Reconfiguring guest network...") return False file = open(jsonConfigFile) @@ -80,7 +80,7 @@ def is_guestnet_configured(guestnet_dict, keys): ''' Guest network has to be removed. ''' - print "[INFO] update_config.py :: Removing guest network..." + logging.info("update_config.py :: Removing guest network...") return False ''' @@ -121,7 +121,10 @@ if jsonFilename != "cmd_line.json" and os.path.isfile(jsonPath % "cmd_line.json" qf.load(None) if not (os.path.isfile(jsonConfigFile) and os.access(jsonConfigFile, os.R_OK)): - print "[ERROR] update_config.py :: Unable to read and access %s to process it" % jsonConfigFile + # Ignore if file is already processed + if os.path.isfile(jsonPath % ("processed/" + jsonFilename + ".gz")): + sys.exit(0) + logging.error("update_config.py :: Unable to read and access %s to process it" % jsonConfigFile) sys.exit(1) # If the guest network is already configured and have the same IP, do not try to configure it again otherwise it will break @@ -131,14 +134,14 @@ if jsonFilename.startswith("guest_network.json"): guestnet_dict = json.load(file) if not is_guestnet_configured(guestnet_dict, ['eth1', 'eth2', 'eth3', 'eth4', 'eth5', 'eth6', 'eth7', 'eth8', 'eth9']): - print "[INFO] update_config.py :: Processing Guest Network." + logging.info("update_config.py :: Processing Guest Network.") process_file() else: - print "[INFO] update_config.py :: No need to process Guest Network." + logging.info("update_config.py :: No need to process Guest Network.") finish_config() else: - print "[INFO] update_config.py :: No GuestNetwork configured yet. Configuring first one now." + logging.info("update_config.py :: No GuestNetwork configured yet. Configuring first one now.") process_file() else: - print "[INFO] update_config.py :: Processing incoming file => %s" % sys.argv[1] + logging.info("update_config.py :: Processing incoming file => %s" % sys.argv[1]) process_file() diff --git a/test/integration/smoke/test_deploy_virtio_scsi_vm.py b/test/integration/smoke/test_deploy_virtio_scsi_vm.py index 260e299d4f9..df54c4307a9 100644 --- a/test/integration/smoke/test_deploy_virtio_scsi_vm.py +++ b/test/integration/smoke/test_deploy_virtio_scsi_vm.py @@ -25,35 +25,30 @@ from marvin.cloudstackTestCase import cloudstackTestCase # base - contains all resources as entities and defines create, delete, # list operations on them from marvin.lib.base import (Account, - VirtualMachine, - ServiceOffering, - NetworkOffering, - Network, - Template, - DiskOffering, - StoragePool, - Volume, - Host, - GuestOs) - - + VirtualMachine, + ServiceOffering, + Template, + DiskOffering, + Volume, + Host, + GuestOs) # utils - utility classes for common cleanup, external library wrappers etc from marvin.lib.utils import cleanup_resources, get_hypervisor_type, validateList # common - commonly used methods for all tests are listed here -from marvin.lib.common import get_zone, get_domain, get_template, list_hosts, get_pod +from marvin.lib.common import get_zone, get_domain, get_pod from marvin.sshClient import SshClient -from marvin.codes import FAILED, PASS +from marvin.codes import FAILED from nose.plugins.attrib import attr import xml.etree.ElementTree as ET -import code import logging + class Templates: """Test data for templates """ @@ -75,11 +70,12 @@ class Templates: } } -class TestDeployVirtioSCSIVM(cloudstackTestCase): +class TestDeployVirtioSCSIVM(cloudstackTestCase): """ Test deploy a kvm virtio scsi template """ + @classmethod def setUpClass(cls): cls.logger = logging.getLogger('TestDeployVirtioSCSIVM') @@ -100,7 +96,6 @@ class TestDeployVirtioSCSIVM(cloudstackTestCase): cls.zone = get_zone(cls.apiclient, testClient.getZoneForTests()) cls.pod = get_pod(cls.apiclient, cls.zone.id) cls.services['mode'] = cls.zone.networktype - cls._cleanup = [] if cls.hypervisor.lower() not in ['kvm']: cls.hypervisorNotSupported = True return @@ -153,41 +148,38 @@ class TestDeployVirtioSCSIVM(cloudstackTestCase): cls.vmhost = hosts[0] - + # Stop VM to reset password + cls.virtual_machine.stop(cls.apiclient) password = cls.virtual_machine.resetPassword(cls.apiclient) cls.virtual_machine.username = "ubuntu" cls.virtual_machine.password = password - cls._cleanup = [ + + # Start VM after password reset + cls.virtual_machine.start(cls.apiclient) + + cls.cleanup = [ cls.template, cls.service_offering, cls.sparse_disk_offering, cls.account ] - @classmethod def tearDownClass(cls): try: + cls.apiclient = super( + TestDeployVirtioSCSIVM, + cls + ).getClsTestClient().getApiClient() # Cleanup resources used - cleanup_resources(cls.apiclient, cls._cleanup) + cleanup_resources(cls.apiclient, cls.cleanup) except Exception as e: raise Exception("Warning: Exception during cleanup : %s" % e) - return def setUp(self): self.apiclient = self.testClient.getApiClient() self.dbclient = self.testClient.getDbConnection() - self.cleanup = [] - return - - def tearDown(self): - try: - # Clean up, terminate the created instance, volumes and snapshots - cleanup_resources(self.apiclient, self.cleanup) - except Exception as e: - raise Exception("Warning: Exception during cleanup : %s" % e) - return def verifyVirshState(self, diskcount): host = self.vmhost.ipaddress @@ -212,14 +204,14 @@ class TestDeployVirtioSCSIVM(cloudstackTestCase): for child in disk: if child.tag.lower() == "target": dev = child.get("dev") - self.assert_(dev != None and dev.startswith("sd"), "disk dev is invalid") + self.assert_(dev is not None and dev.startswith("sd"), "disk dev is invalid") elif child.tag.lower() == "address": con = child.get("controller") self.assertEqual(con, scsiindex, "disk controller not equal to SCSI " \ - "controller index") + "controller index") elif child.tag.lower() == "driver": discard = child.get("discard") - if discard: # may not be defined by older qemu/libvirt + if discard: # may not be defined by older qemu/libvirt self.assertEqual(discard, "unmap", "discard settings not unmap") def verifyGuestState(self, diskcount): @@ -234,21 +226,21 @@ class TestDeployVirtioSCSIVM(cloudstackTestCase): "Could not find appropriate number of scsi disks in guest") def getVirshXML(self, host, instancename): - if host == None: + if host is None: self.logger.debug("getVirshXML: host is none") return "" else: self.logger.debug("host is: " + host) - if instancename == None: + if instancename is None: self.logger.debug("getVirshXML: instancename is none") return "" else: self.logger.debug("instancename is: " + instancename) sshc = SshClient( - host=host, - port=self.services['configurableData']['host']["publicport"], - user=self.hostConfig['username'], - passwd=self.hostConfig['password']) + host=host, + port=self.services['configurableData']['host']["publicport"], + user=self.hostConfig['username'], + passwd=self.hostConfig['password']) ssh = sshc.ssh @@ -354,9 +346,8 @@ class TestDeployVirtioSCSIVM(cloudstackTestCase): self.assertIsNotNone(ostypeid, "Could not find ostypeid for Ubuntu 16.0.4 (64-bit) mapped to kvm") - self.virtual_machine.update(self.apiclient, ostypeid=ostypeid, - details=[{"rootDiskController":"scsi"}]) + details=[{"rootDiskController": "scsi"}]) self.virtual_machine.start(self.apiclient) @@ -371,6 +362,7 @@ class TestDeployVirtioSCSIVM(cloudstackTestCase): self.verifyGuestState(3) + class CommandNonzeroException(Exception): def __init__(self, code, stderr): self.code = code diff --git a/test/integration/smoke/test_vm_life_cycle.py b/test/integration/smoke/test_vm_life_cycle.py index e9970b6f212..5906a94869c 100644 --- a/test/integration/smoke/test_vm_life_cycle.py +++ b/test/integration/smoke/test_vm_life_cycle.py @@ -918,7 +918,7 @@ class TestSecuredVmMigration(cloudstackTestCase): cmd = provisionCertificate.provisionCertificateCmd() cmd.hostid = host.id cmd.reconnect = True - self.apiclient.updateConfiguration(cmd) + self.apiclient.provisionCertificate(cmd) for host in self.hosts: self.check_connection(secured='true', host=host)