PR#50: replace arp with libvirt method - testcloud

testcloud

#50 replace arp with libvirt method

Merged 6 years ago by roshi. Opened 6 years ago by kparal.

feature/noarp into dev

replace arp with libvirt method

Kamil Páral • 6 years ago

e35e465

testcloud/cli.py

file modified

+2 -42

		`@@ -10,13 +10,11 @@`

		`import argparse`
		`import logging`
		`- from time import sleep`
		`import os`
		`from . import config`
		`from . import image`
		`from . import instance`
		`- from . import util`
		`- from .exceptions import DomainNotFoundError, TestcloudCliError, TestcloudInstanceError`
		`+ from .exceptions import TestcloudCliError`

		`config_data = config.get_config()`

		`@@ -97,7 +95,7 @@`
		`tc_instance.start(args.timeout)`

		`# find vm ip`
		`- vm_ip = find_vm_ip(args.name, args.connection)`
		`+ vm_ip = tc_instance.get_ip()`

		`# Write ip to file`
		`tc_instance.create_ip_file(vm_ip)`
		`@@ -341,41 +339,3 @@`
		`_configure_logging()`

		`args.func(args)`
		`-`
		`-`
		`- def find_vm_ip(name, connection='qemu:///system'):`
		`- """Finds the ip of a local vm given it's name used by libvirt.`
		`-`
		`- :param str name: name of the VM (as used by libvirt)`
		`- :param str connection: name of the libvirt connection uri`
		`- :returns: ip address of VM`
		`- :rtype: str`
		`- """`
		`-`
		`- for _ in xrange(100):`
		`- vm_xml = util.get_vm_xml(name, connection)`
		`- if vm_xml is not None:`
		`- break`
		`-`
		`- else:`
		`- sleep(.2)`
		`- else:`
		`- raise DomainNotFoundError`
		`-`
		`- vm_mac = util.find_mac(vm_xml)`
		`- vm_mac = vm_mac[0]`
		`-`
		`- # The arp cache takes some time to populate, so this keeps looking`
		`- # for the entry until it shows up.`
		`-`
		`- for _ in xrange(100):`
		`- vm_ip = util.find_ip_from_mac(vm_mac.attrib['address'])`
		`-`
		`- if vm_ip:`
		`- break`
		`-`
		`- sleep(.2)`
		`- else:`
		`- raise TestcloudInstanceError('Could not find VM\'s ip before timeout')`
		`-`
		`- return vm_ip`

testcloud/instance.py

file modified

+43

		`@@ -505,3 +505,46 @@`

		`log.debug('DEPRECATED: destroy() method was deprecated. Please use remove()')`
		`self.remove()`
		`+`
		`+ def get_ip(self, timeout=60):`
		`+ '''Retrieve IP address of the instance (the first one, if there are`
		`+ multiple).`
		`+`
		`+ :param int timeout: how long to wait if IP address is not yet ready`
		`+ (e.g. when booting), in seconds`
		`+ :return: IP address of the instance`
		`+ :rtype: str`
		`+ :raises TestcloudInstanceError: when time runs out and no IP is`
		`+ assigned`
		`+ '''`
		`+`
		`+ dom = self._get_domain()`
		`+ counter = 0`
		`+ sleep_interval = 0.2`
		`+`
		`+ while counter <= timeout:`
		`+ try:`
		`+ output = dom.interfaceAddresses(`
		`+ libvirt.VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE)`
		`+ # example output:`
		`+ # {'vnet0': {'addrs': [{'addr': '192.168.11.33', 'prefix': 24, 'type': 0}],`
		`+ # 'hwaddr': '52:54:00:54:4b:b4'}}`
		`+ if output:`
		`+ addrs = [ addr['addr'] for iface in output.values()`
		`+ for addr in iface.get('addrs', [])`
		`+ if 'addr' in addr ]`
		`+ if addrs:`
		`+ return addrs[0]`
		`+ except libvirt.libvirtError as e:`
		`+ if e.get_error_code() == libvirt.VIR_ERR_OPERATION_INVALID:`
		`+ # the domain is not yet running`
		`+ pass`
		`+ else:`
		`+ raise e`
		`+ counter += sleep_interval`
		`+ time.sleep(sleep_interval)`
		`+`
		`+ msg = "Couldn't find IP for %s before %s second timeout" % (self.name,`
		`+ timeout)`
		`+ log.warn(msg)`
		`+ raise TestcloudInstanceError(msg)`

testcloud/util.py

file modified

-41

		`@@ -8,12 +8,8 @@`
		`This module contains helper functions for testcloud.`
		`"""`

		`- import subprocess`
		`import logging`
		`-`
		`import random`
		`- import libvirt`
		`- import xml.etree.ElementTree as ET`

		`from . import config`

		`@@ -21,43 +17,6 @@`
		`config_data = config.get_config()`


		`- def get_vm_xml(instance_name, connection='qemu:///system'):`
		`- """Query virsh for the xml of an instance by name."""`
		`-`
		`- con = libvirt.openReadOnly(connection)`
		`- try:`
		`- domain = con.lookupByName(instance_name)`
		`-`
		`- except libvirt.libvirtError:`
		`- return None`
		`-`
		`- result = domain.XMLDesc()`
		`-`
		`- return str(result)`
		`-`
		`-`
		`- def find_mac(xml_string):`
		`- """Pass in a virsh xmldump and return a list of any mac addresses listed.`
		`- Typically it will just be one.`
		`- """`
		`-`
		`- xml_data = ET.fromstring(xml_string)`
		`-`
		`- macs = xml_data.findall("./devices/interface/mac")`
		`-`
		`- return macs`
		`-`
		`-`
		`- def find_ip_from_mac(mac):`
		- """Look through ``arp -an`` output for the IP of the provided MAC address.
		`- """`
		`-`
		`- arp_list = subprocess.check_output(["arp", "-an"]).split("\n")`
		`- for entry in arp_list:`
		`- if mac in entry:`
		`- return entry.split()[1][1:-1]`
		`-`
		`-`
		`def generate_mac_address():`
		`"""Create a workable mac address for our instances."""`

kparal commented 6 years ago

net-tools (containing arp) is getting obsoleted. Instead of using arp,
use native libvirt methods for querying the IP address.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1496158

Tested, works for me. @roshi @lbrabec please review and test as well. Once this is pushed, we'll also need to create a new release.

tflink commented 6 years ago

Something in the back of my mind is remembering that this method has some requirement on the OS inside the VM. @roshi, do you remember any more about why you chose to use arp instead of another method?

lbrabec commented 6 years ago

LGTM, ack!

kparal commented 6 years ago

@tflink The code should do the same as virsh domifaddr domain --source lease:

   domifaddr domain [interface] [--full] [--source lease|agent]
       Get a list of interfaces of a running domain along with their IP and MAC addresses, or limited output
       just for one interface if interface is specified. Note that interface can be driver dependent, it can be
       the name within guest OS or the name you would see in domain XML. Moreover, the whole command may
       require a guest agent to be configured for the queried domain under some drivers, notably qemu. If
       --full is specified, the interface name is always displayed when the interface has multiple addresses or
       alias, otherwise it only displays the interface name for the first address, and "-" for the others. The
       --source argument specifies what data source to use for the addresses, currently one of 'lease' to read
       DHCP leases, or 'agent' to query the guest OS via an agent. If unspecified, 'lease' is the default.

So it should look into DHCP leases (similar to arp functionality) and not require any agent.

tflink commented 6 years ago

From the doc you quoted:

Moreover, the whole command may require a guest agent to be configured for the queried domain under some drivers, notably qemu.

roshi commented 6 years ago

Overall, LGTM. I was using arp to find the IP because it seemed the least likely to break and I knew it would never rely on agents being installed on the guest. It might be worth it to port the arp code to using iproute2, at least as a fallback.

tflink commented 6 years ago

I'm a little concerned about the possibility of qemu's guest agent causing problems but I'm not arguing that we dont' have to move off of arp.

It's an improvement and we can deal with fallout if/when it happens. ACK