Skip to contentSkip to conversation

TAP support (and a few other small things)

PATCH In review Michael Goldish 5 participants 28 messages Aug 2, 2009 – Aug 7, 2009 KVM

lore
Share & tools
lore
#0 lore raw

cover TAP support (and a few other small things)

Here's my proposed TAP solution:
- The user specifies MAC ranges and may or may not specify corresponding IP ranges.
- VMs are always given MAC addresses from the user specified MAC ranges.
- MAC address ranges must be unique to each host running KVM tests -- there must be no overlap between them.
- If corresponding IP ranges are specified as well, the system will use them when trying to communicate with guests.
- If corresponding IP ranges are not specified, tcpdump will be used to listen to DHCP traffic and dynamically detect the right IP addresses. There's also a flag that can force this behavior.
- tcpdump will run in the background at all times, and collect MAC-IP pairs as soon as they are assigned by the DHCP server. This is useful for NIC hotplugging (where IP addresses are assigned during the test itself) and generally for misbehaving guests or DHCP servers (restarting network services during the test, using very short lease times).
- It is up to the user to create the actual bridge for TAP devices; the qemu-ifup script included in one of the patches only adds the TAP interface to an existing bridge. The user should modify this script or create a bridge some other way.

This works very well on two hosts I tried, but I'm not entirely sure it will work in all cases -- please let me know what you think.

(I remember Jason Wang mentioned that tcpdump doesn't always catch all the required traffic, or that it might somehow depend on the DHCP server -- anyone willing to comment on that?)

The following patches have been tested with a few KVM tests (boot, reboot, stress_boot, migration) with both TAP and user mode, but they could probably use more testing (maybe with Python 2.6?), so if anyone is willing to help with that I'd appreciate it very much.

#1 lore raw

PATCH 1/12 KVM test: add some MAC/IP address utilities to kvm_utils

Add function get_mac_ip_pair_from_dict() which gets a dict (specified in the config file by the user) and fetches a MAC-IP address pair according to a certain syntax. The syntax allows the user to specify a group of MAC-IP address ranges. For example:

address_ranges = r1 r2 r3

address_range_base_mac_r1 = 55:44:33:22:11:00
address_range_base_ip_r1 = 10.0.0.0
address_range_size_r1 = 16

address_range_base_mac_r2 = 55:44:33:22:11:40
address_range_base_ip_r2 = 10.0.0.60
address_range_size_r2 = 25

address_range_base_mac_r3 = 55:44:33:22:12:10
address_range_base_ip_r3 = 10.0.1.20
address_range_size_r3 = 230

The lines above may be specified globally, so that they apply to all VMs and all NICs.

However, a line similar to the following must be specified per NIC (each VM may have several NICs):

address_index = 0

Currently, we usually use a single VM and a single NIC, so specifying address_index once should suffice. If a test requires an additional NIC the user should add something like:

address_index_nic2 = 1

The index is simply the index in the MAC-IP table that consists of all the specified ranges. In the above example, if the user specifies an index of 18, the MAC-IP pair will be taken from the second range, because the first range has only 16 entries.

When running migration, both the source and destination VMs should have the same address_index, because they should have the same MAC and IP addresses.

Note that different copies of the KVM test, running simultaneously in the same network environment, _must_ specify different MAC-IP pools. This can be done in several ways:
- By specifying the ranges (as in the example above) in an external file such as /etc/kvm-autotest/client_mac_ip_pool.cfg, and setting up that file for each host manually
- By keeping several .cfg files with different names that match the hostname of each host, e.g. mac_ip_pool_hostname1.cfg, mac_ip_pool_hostname2.cfg (hostname1 and hostname2 should be replaced by actual hostnames), and parsing the right file in the control file at runtime
- By defining all the different pools in a variants block in a single file, and specifying 'only <hostname>' at runtime in the control file (using config.parse_string())
When we start running in server mode, assigning MAC and IP addresses to hosts can be done automatically by the server, but the user will still be required to specify a single global pool for the server (which the server will divide among the hosts). The address_index parameter will be specified inside the regular config file, and does not need to be different for each host.

This patch also adds some small utility functions used by get_mac_ip_pair_from_dict().

---
 client/tests/kvm/kvm_utils.py |  110 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 108 insertions(+), 2 deletions(-)
client/tests/kvm/kvm_utils.py +108 -2
index e897e78..5d8291f 100644
@@ -1,7 +1,7 @@
 import md5, thread, subprocess, time, string, random, socket, os, signal, pty
 import select, re, logging
-from autotest_lib.client.bin import utils
-from autotest_lib.client.common_lib import error
+#from autotest_lib.client.bin import utils
+#from autotest_lib.client.common_lib import error
 import kvm_subprocess
 
 """
@@ -46,6 +46,112 @@ def get_sub_dict_names(dict, keyword):
         return []
 
 
+# Functions related to MAC/IP addresses
+
+def mac_str_to_int(addr):
+    """
+    Convert MAC address string to integer.
+
+    @param addr: String representing the MAC address.
+    """
+    return sum(int(s, 16) * 256 ** i
+               for i, s in enumerate(reversed(addr.split(":"))))
+
+
+def mac_int_to_str(addr):
+    """
+    Convert MAC address integer to string.
+
+    @param addr: Integer representing the MAC address.
+    """
+    return ":".join("%02x" % (addr >> 8 * i & 0xFF)
+                    for i in reversed(range(6)))
+
+
+def ip_str_to_int(addr):
+    """
+    Convert IP address string to integer.
+
+    @param addr: String representing the IP address.
+    """
+    return sum(int(s) * 256 ** i
+               for i, s in enumerate(reversed(addr.split("."))))
+
+
+def ip_int_to_str(addr):
+    """
+    Convert IP address integer to string.
+
+    @param addr: Integer representing the IP address.
+    """
+    return ".".join(str(addr >> 8 * i & 0xFF)
+                    for i in reversed(range(4)))
+
+
+def offset_mac(base, offset):
+    """
+    Add offset to a given MAC address.
+
+    @param base: String representing a MAC address.
+    @param offset: Offset to add to base (integer)
+    @return: A string representing the offset MAC address.
+    """
+    return mac_int_to_str(mac_str_to_int(base) + offset)
+
+
+def offset_ip(base, offset):
+    """
+    Add offset to a given IP address.
+
+    @param base: String representing an IP address.
+    @param offset: Offset to add to base (integer)
+    @return: A string representing the offset IP address.
+    """
+    return ip_int_to_str(ip_str_to_int(base) + offset)
+
+
+def get_mac_ip_pair_from_dict(dict):
+    """
+    Fetch a MAC-IP address pair from dict and return it.
+
+    The parameters in dict are expected to conform to a certain syntax.
+    Typical usage may be:
+
+    address_ranges = r1 r2 r3
+
+    address_range_base_mac_r1 = 55:44:33:22:11:00
+    address_range_base_ip_r1 = 10.0.0.0
+    address_range_size_r1 = 16
+
+    address_range_base_mac_r2 = 55:44:33:22:11:40
+    address_range_base_ip_r2 = 10.0.0.60
+    address_range_size_r2 = 25
+
+    address_range_base_mac_r3 = 55:44:33:22:12:10
+    address_range_base_ip_r3 = 10.0.1.20
+    address_range_size_r3 = 230
+
+    address_index = 0
+
+    All parameters except address_index specify a MAC-IP address pool.  The
+    pool consists of several MAC-IP address ranges.
+    address_index specified the index of the desired MAC-IP pair from the pool.
+
+    @param dict: The dictionary from which to fetch the addresses.
+    """
+    index = int(dict.get("address_index", 0))
+    for mac_range_name in get_sub_dict_names(dict, "address_ranges"):
+        mac_range_params = get_sub_dict(dict, mac_range_name)
+        mac_base = mac_range_params.get("address_range_base_mac")
+        ip_base = mac_range_params.get("address_range_base_ip")
+        size = int(mac_range_params.get("address_range_size", 1))
+        if index < size:
+            return (mac_base and offset_mac(mac_base, index),
+                    ip_base and offset_ip(ip_base, index))
+        index -= size
+    return (None, None)
+
+
 # Functions for working with the environment (a dict-like object)
 
 def is_vm(obj):
Signature · 1 line
1.5.4.1
#2 lore raw

PATCH 2/12 KVM test: add basic TAP support with static MAC-IP mapping

The network mode is specified by a NIC parameter named 'nic_mode'. Currently two modes are supported: 'tap' and 'user'.

Other NIC parameters are now handled: nic_script, nic_downscript and nic_ifname. nic_ifname should usually not be specified (qemu chooses one automatically). nic_script and nic_downscript may specify a relative path, e.g. 'qemu-ifup', which refers to a file in the KVM test local dir, or an absolute path, e.g. '/etc/qemu-ifup'.

This patch mainly modifies VM.get_address(), VM.get_port() and VM.make_qemu_command(), and adds a 'script_dir' parameter to several VM routines. 'script_dir' should normally be the KVM test local dir.

---
 client/tests/kvm/kvm_preprocessing.py |    8 ++-
 client/tests/kvm/kvm_tests.cfg.sample |    6 ++
 client/tests/kvm/kvm_vm.py            |   95 +++++++++++++++++++++++---------
 3 files changed, 79 insertions(+), 30 deletions(-)
client/tests/kvm/kvm_preprocessing.py +5 -3
index d118826..4d77630 100644
@@ -46,6 +46,7 @@ def preprocess_vm(test, params, env, name):
     qemu_path = os.path.join(test.bindir, "qemu")
     image_dir = os.path.join(test.bindir, "images")
     iso_dir = os.path.join(test.bindir, "isos")
+    script_dir = test.bindir
 
     logging.debug("Preprocessing VM '%s'..." % name)
     vm = kvm_utils.env_get_vm(env, name)
@@ -53,7 +54,7 @@ def preprocess_vm(test, params, env, name):
         logging.debug("VM object found in environment")
     else:
         logging.debug("VM object does not exist; creating it")
-        vm = kvm_vm.VM(name, params, qemu_path, image_dir, iso_dir)
+        vm = kvm_vm.VM(name, params, qemu_path, image_dir, iso_dir, script_dir)
         kvm_utils.env_register_vm(env, name, vm)
 
     start_vm = False
@@ -74,14 +75,15 @@ def preprocess_vm(test, params, env, name):
         elif vm.make_qemu_command() != vm.make_qemu_command(name, params,
                                                             qemu_path,
                                                             image_dir,
-                                                            iso_dir):
+                                                            iso_dir,
+                                                            script_dir):
             logging.debug("VM's qemu command differs from requested one; "
                           "restarting it...")
             start_vm = True
 
     if start_vm:
         if not vm.create(name, params, qemu_path, image_dir, iso_dir,
-                         for_migration):
+                         script_dir, for_migration):
             message = "Could not start VM"
             logging.error(message)
             raise error.TestError(message)
client/tests/kvm/kvm_tests.cfg.sample +6 -0
index 7cd12cb..4b9d7eb 100644
@@ -24,6 +24,12 @@ display = vnc
 redirs = ssh
 guest_port_ssh = 22
 
+# NIC parameters
+nic_mode = tap
+#nic_mode = user
+nic_script = qemu-ifup
+address_index = 0
+
 
 # Tests
 variants:
client/tests/kvm/kvm_vm.py +68 -27

217 lines not on this page · raw message

Signature · 1 line
1.5.4.1
#3 lore raw

PATCH 3/12 KVM test: add sample 'qemu-ifup' script

The script adds a requested interface to an existing bridge. It is meant to be used by qemu when running in TAP mode.

Note: the user is responsible for setting up the bridge before running any tests. This can be done with brctl or in any manner that is appropriate for the host OS. It can be done inside 'qemu-ifup' as well, but this sample script doesn't do it.

---
 client/tests/kvm/qemu-ifup |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
 create mode 100644 client/tests/kvm/qemu-ifup
client/tests/kvm/qemu-ifup new file +8 -0
new file mode 100644
index 0000000..bcd9a7a
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+# The following expression selects the first bridge listed by 'brctl show'.
+# Modify it to suit your needs.
+switch=$(/usr/sbin/brctl show | awk 'NR==2 { print $1 }')
+
+/sbin/ifconfig $1 0.0.0.0 up
+/usr/sbin/brctl addif ${switch} $1
Signature · 1 line
1.5.4.1
#4 lore raw

PATCH 4/12 KVM test: add host-specific MAC-IP pool configuration

- Add kvm_address_pools.cfg which specifies a set of MAC-IP ranges for each host.
- Read kvm_address_pools.cfg in the control file and filter it by hostname, so that each host uses only the MAC addresses assigned to it. If the hostname doesn't match any hostname in kvm_address_pools.cfg, use 'default_host'.
- While we're at it, add a comment informing the user where to modify the test configration, if that is desirable
- (Also add a missing space somewhere in the control file, and remove a few extra ones)

---
 client/tests/kvm/control                      |   25 +++++++++--
 client/tests/kvm/kvm_address_pools.cfg.sample |   56 +++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 5 deletions(-)
 create mode 100644 client/tests/kvm/kvm_address_pools.cfg.sample
client/tests/kvm/control +20 -5
index 5c5d424..2ad8d81 100644
@@ -133,12 +133,27 @@ if not r:
 import kvm_config
 
 filename = os.path.join(pwd, "kvm_tests.cfg")
-list = kvm_config.config(filename).get_list()
+cfg = kvm_config.config(filename)
+
+# If desirable, make changes to the test configuration here.  For example:
+# cfg.parse_string("only fc8_quick")
+# cfg.parse_string("display = sdl")
+
+filename = os.path.join(pwd, "kvm_address_pools.cfg")
+if os.path.exists(filename):
+    cfg.parse_file(filename)
+    hostname = os.uname()[1].split(".")[0]
+    if cfg.filter("^" + hostname):
+        cfg.parse_string("only ^%s" % hostname)
+    else:
+        cfg.parse_string("only ^default_host")
+
+list = cfg.get_list()
 
 
-#  -------------
-#  Run the tests
-#  -------------
+# -------------
+# Run the tests
+# -------------
 status_dict = {}
 
 for dict in list:
@@ -153,7 +168,7 @@ for dict in list:
                 dependencies_satisfied = False
                 break
     if dependencies_satisfied:
-        test_iterations=int(dict.get("iterations", 1))
+        test_iterations = int(dict.get("iterations", 1))
         current_status = job.run_test("kvm", params=dict,
                                       tag=dict.get("shortname"),
                                       iterations=test_iterations)
client/tests/kvm/kvm_address_pools.cfg.sample new file +56 -0

56 lines not on this page · raw message

Signature · 1 line
1.5.4.1
#5 lore raw

PATCH 5/12 KVM test: specify installation cdkeys in a dedicated file

- Add file kvm_cdkeys.cfg.sample which specifies cdkeys for guests that require them. Users should rename the file to kvm_cdkeys.cfg and enter the real cdkeys in the file.

- Remove all cdkey lines from kvm_tests.cfg.sample and add an include statement instead ('include kvm_cdkeys.cfg').

---
 client/tests/kvm/kvm_cdkeys.cfg.sample |   16 ++++++++++++++++
 client/tests/kvm/kvm_tests.cfg.sample  |   10 +++-------
 2 files changed, 19 insertions(+), 7 deletions(-)
 create mode 100644 client/tests/kvm/kvm_cdkeys.cfg.sample
client/tests/kvm/kvm_cdkeys.cfg.sample new file +16 -0
new file mode 100644
index 0000000..60bfe59
@@ -0,0 +1,16 @@
+# Rename this file to kvm_cdkeys.cfg.
+# Replace the 'CDKEY' strings with real cdkeys where necessary.
+# Guests that do not require cdkeys by default are commented out.
+# You may uncomment them if necessary.
+
+RHEL.5.3.i386: cdkey = CDKEY
+RHEL.5.3.x86_64: cdkey = CDKEY
+Win2000: cdkey = CDKEY
+WinXP.32: cdkey = CDKEY
+WinXP.64: cdkey = CDKEY
+Win2003.32: cdkey = CDKEY
+Win2003.64: cdkey = CDKEY
+#WinVista.32: cdkey = CDKEY
+#WinVista.64: cdkey = CDKEY
+#Win2008.32: cdkey = CDKEY
+#Win2008.64: cdkey = CDKEY
client/tests/kvm/kvm_tests.cfg.sample +3 -7
index 4b9d7eb..3c23432 100644
@@ -272,7 +272,6 @@ variants:
                         image_name = rhel5-32
                         install:
                             steps=RHEL-5.3-i386.steps
-                            cdkey=RHEL_INSTALL_NUMBER
                             cdrom=linux/RHEL-5.3-i386-DVD.iso
                             md5sum=371c62851611fd32ead440df6f24a296
                             md5sum_1m=242318dd44152210f6ff6cdda1bfbf51
@@ -282,7 +281,6 @@ variants:
                         image_name = rhel5-64
                         install:
                             steps=RHEL-5.3-x86_64.steps
-                            cdkey=RHEL_INSTALL_NUMBER
                             cdrom=linux/RHEL-5.3-x86_64-DVD.iso
                             md5sum=c5ed6b284410f4d8212cafc78fd7a8c5
                             md5sum_1m=b999f437583098ea5bbd56fb1de1d011
@@ -363,7 +361,6 @@ variants:
                     md5sum = dda6039f3a9173f0f6bfae40f5efdfea
                     md5sum_1m = dd28fba196d366d56fe774bd93df5527
                     user = user
-                    cdkey = WIN_2000_32_CDKEY
                 setup:
                     steps = Win2000-32-setupssh.steps
                     cdrom = windows/setupssh.iso
@@ -376,7 +373,6 @@ variants:
                     md5sum = 743450644b1d9fe97b3cf379e22dceb0
                     md5sum_1m = b473bf75af2d1269fec8958cf0202bfd
                     user = user
-                    cdkey = WIN_XP_32_CDKEY
                 setup:
                     steps = WinXP-32-setupssh.steps
                     cdrom = windows/setupssh.iso
@@ -390,7 +386,6 @@ variants:
                     md5sum = 8d3f007ec9c2060cec8a50ee7d7dc512
                     md5sum_1m = e812363ff427effc512b7801ee70e513
                     user = user
-                    cdkey = WIN_XP_64_CDKEY
 
             - Win2003:
                 image_size = 20G
@@ -406,7 +401,6 @@ variants:
                             md5sum = 03e921e9b4214773c21a39f5c3f42ef7
                             md5sum_1m = 37c2fdec15ac4ec16aa10fdfdb338aa3
                             user = user
-                            cdkey = WIN_2003_32_CDKEY
                         setup:
                             steps = Win2003-32-setupssh.steps
                             cdrom = windows/setupssh.iso
@@ -420,7 +414,6 @@ variants:
                             md5sum = 5703f87c9fd77d28c05ffadd3354dbbd
                             md5sum_1m = 439393c384116aa09e08a0ad047dcea8
                             user = user
-                            cdkey = WIN_2003_64_CDKEY
 
             - WinVista:
                 only install
@@ -624,5 +617,8 @@ variants:
         only rtl8139
 
 
+include kvm_cdkeys.cfg
+
+
 # Choose your test list
 only fc8_quick
Signature · 1 line
1.5.4.1
#6 lore raw

PATCH 6/12 KVM test: minor style changes to kvm_config.py

- Fix indentation of a few lines.
- Limit lines to 80 characters.
- Use list comprehension in config.filter().
- Replace '== None' with 'is None'.

---
 client/tests/kvm/kvm_config.py |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)
client/tests/kvm/kvm_config.py +18 -16
index 7e8b1db..e478a55 100755
@@ -83,8 +83,8 @@ class config:
         @param filter: A regular expression that defines the filter.
         @param dict: Dictionary that will be inspected.
         """
-        filter = re.compile("(\\.|^)" + filter + "(\\.|$)")
-        return filter.search(dict["name"]) != None
+        filter = re.compile(r"(\.|^)" + filter + r"(\.|$)")
+        return bool(filter.search(dict["name"]))
 
 
     def filter(self, filter, list=None):
@@ -94,13 +94,9 @@ class config:
         @param filter: A regular expression that will be used as a filter.
         @param list: A list of dictionaries that will be filtered.
         """
-        if list == None:
+        if list is None:
             list = self.list
-        filtered_list = []
-        for dict in list:
-            if self.match(filter, dict):
-                filtered_list.append(dict)
-        return filtered_list
+        return [dict for dict in list if self.match(filter, dict)]
 
 
     # Currently unused, will be removed if it remains unused
@@ -111,7 +107,7 @@ class config:
         @param filter: A regular expression that will filter the list.
         @param list: List which we want to know the indexes that match a filter.
         """
-        if list == None:
+        if list is None:
             list = self.list
         block_list = []
         prev_match = False
@@ -248,17 +244,22 @@ class config:
                 temp_list.append(new_dict)
 
             if subvariants:
-                # If we're parsing 'subvariants', we need to modify the list first
-                self.__modify_list_subvariants(temp_list, name, dep_list, add_to_shortname)
+                # If we're parsing 'subvariants', first modify the list
+                self.__modify_list_subvariants(temp_list, name, dep_list,
+                                               add_to_shortname)
                 temp_list = self.parse(file, temp_list,
                         restricted=True, prev_indent=indent)
             else:
-                # If we're parsing 'variants', we need to parse first and then modify the list
+                # If we're parsing 'variants', parse before modifying the list
                 if self.debug:
-                    self.__debug_print(indented_line, "Entering variant '%s' (variant inherits %d dicts)" % (name, len(list)))
+                    self.__debug_print(indented_line,
+                                       "Entering variant '%s' "
+                                       "(variant inherits %d dicts)" %
+                                       (name, len(list)))
                 temp_list = self.parse(file, temp_list,
                         restricted=False, prev_indent=indent)
-                self.__modify_list_variants(temp_list, name, dep_list, add_to_shortname)
+                self.__modify_list_variants(temp_list, name, dep_list,
+                                            add_to_shortname)
 
             new_list += temp_list
 
@@ -401,7 +402,7 @@ class config:
                     continue
                 if self.debug and not restricted:
                     self.__debug_print(indented_line,
-                                     "Entering file %s" % words[1])
+                                       "Entering file %s" % words[1])
                 if self.filename:
                     filename = os.path.join(os.path.dirname(self.filename),
                                             words[1])
@@ -440,7 +441,8 @@ class config:
                         # Everything inside an exception should be parsed in
                         # restricted mode
                         new_list += self.parse(file, list[index:index+1],
-                                restricted=True, prev_indent=indent)
+                                               restricted=True,
+                                               prev_indent=indent)
                     else:
                         new_list += list[index:index+1]
                 list = new_list
Signature · 1 line
1.5.4.1
#7 lore raw

PATCH 7/12 KVM test: kvm_config.py: do not fail when including a nonexistent file

Instead of failing, just print a warning. Included files may not always be crucial for tests to run (kvm_cdkeys.cfg for example).

---
 client/tests/kvm/kvm_config.py |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)
client/tests/kvm/kvm_config.py +11 -10
index e478a55..7ff7a07 100755
@@ -406,17 +406,18 @@ class config:
                 if self.filename:
                     filename = os.path.join(os.path.dirname(self.filename),
                                             words[1])
-                    if not os.path.exists(filename):
-                        e_msg = "Cannot include %s -- file not found" % filename
-                        raise error.AutotestError(e_msg)
-                    new_file = open(filename, "r")
-                    list = self.parse(new_file, list, restricted)
-                    new_file.close()
-                    if self.debug and not restricted:
-                        self.__debug_print("", "Leaving file %s" % words[1])
+                    if os.path.exists(filename):
+                        new_file = open(filename, "r")
+                        list = self.parse(new_file, list, restricted)
+                        new_file.close()
+                        if self.debug and not restricted:
+                            self.__debug_print("", "Leaving file %s" % words[1])
+                    else:
+                        print ("WARNING: Cannot include %s -- "
+                               "file not found" % filename)
                 else:
-                    e_msg = "Cannot include anything because no file is open"
-                    raise error.AutotestError(e_msg)
+                    print ("WARNING: Cannot include %s because no file is "
+                           "currently open" % words[1])
 
             # Parse multi-line exceptions
             # (the block is parsed for each dict separately)
Signature · 1 line
1.5.4.1
#8 lore raw

PATCH 8/12 KVM test: kvm_subprocess: send user specified parameters to kvm_tail callbacks

Allow the user to specify parameters to send to output_func and termination_func in addition to the regular output string and exit status. The user specified parameters will be pre-pended to the other parameters. This is mainly meant to bypass Python's refusal to pickle instance methods.

---
 client/tests/kvm/kvm_subprocess.py |   54 ++++++++++++++++++++++++++++++++---
 1 files changed, 49 insertions(+), 5 deletions(-)
client/tests/kvm/kvm_subprocess.py +49 -5
index c15e779..df1d562 100644
@@ -464,7 +464,9 @@ class kvm_tail(kvm_spawn):
     """
 
     def __init__(self, command=None, id=None, echo=False, linesep="\n",
-                 termination_func=None, output_func=None, output_prefix=""):
+                 termination_func=None, termination_params=(),
+                 output_func=None, output_params=(),
+                 output_prefix=""):
         """
         Initialize the class and run command as a child process.
 
@@ -479,10 +481,14 @@ class kvm_tail(kvm_spawn):
                 child process by sendline().
         @param termination_func: Function to call when the process exits.  The
                 function must accept a single exit status parameter.
+        @param termination_params: Parameters to send to termination_func
+                before the exit status.
         @param output_func: Function to call whenever a line of output is
                 available from the STDOUT or STDERR streams of the process.
                 The function must accept a single string parameter.  The string
                 does not include the final newline.
+        @param output_params: Parameters to send to output_func before the
+                output line.
         @param output_prefix: String to prepend to lines sent to output_func.
         """
         # Add a reader and a close hook
@@ -494,7 +500,9 @@ class kvm_tail(kvm_spawn):
 
         # Remember some attributes
         self.termination_func = termination_func
+        self.termination_params = termination_params
         self.output_func = output_func
+        self.output_params = output_params
         self.output_prefix = output_prefix
 
         # Start the thread in the background
@@ -504,7 +512,9 @@ class kvm_tail(kvm_spawn):
 
     def __getinitargs__(self):
         return kvm_spawn.__getinitargs__(self) + (self.termination_func,
+                                                  self.termination_params,
                                                   self.output_func,
+                                                  self.output_params,
                                                   self.output_prefix)
 
 
@@ -518,6 +528,16 @@ class kvm_tail(kvm_spawn):
         self.termination_func = termination_func
 
 
+    def set_termination_params(self, termination_params):
+        """
+        Set the termination_params attribute. See __init__() for details.
+
+        @param termination_params: Parameters to send to termination_func
+                before the exit status.
+        """
+        self.termination_params = termination_params
+
+
     def set_output_func(self, output_func):
         """
         Set the output_func attribute. See __init__() for details.
@@ -528,6 +548,16 @@ class kvm_tail(kvm_spawn):
         self.output_func = output_func
 
 
+    def set_output_params(self, output_params):
+        """
+        Set the output_params attribute. See __init__() for details.
+
+        @param output_params: Parameters to send to output_func before the
+                output line.
+        """
+        self.output_params = output_params
+
+
     def set_output_prefix(self, output_prefix):
         """
         Set the output_prefix attribute. See __init__() for details.
@@ -546,7 +576,8 @@ class kvm_tail(kvm_spawn):
             text = text.decode("utf-8", "replace")
             # Pass it to output_func
             try:
-                self.output_func(text)
+                params = self.output_params + (text,)
+                self.output_func(*params)
             except TypeError:
                 pass
 
@@ -587,7 +618,8 @@ class kvm_tail(kvm_spawn):
             return
         print_line("(Process terminated with status %s)" % status)
         try:
-            self.termination_func(status)
+            params = self.termination_params + (status,)
+            self.termination_func(*params)
         except TypeError:
             pass
 
@@ -607,7 +639,9 @@ class kvm_expect(kvm_tail):
     """
 
     def __init__(self, command=None, id=None, echo=False, linesep="\n",
-                 termination_func=None, output_func=None, output_prefix=""):
+                 termination_func=None, termination_params=(),
+                 output_func=None, output_params=(),
+                 output_prefix=""):
         """
         Initialize the class and run command as a child process.
 
@@ -622,10 +656,14 @@ class kvm_expect(kvm_tail):
                 child process by sendline().
         @param termination_func: Function to call when the process exits.  The
                 function must accept a single exit status parameter.
+        @param termination_params: Parameters to send to termination_func
+                before the exit status.
         @param output_func: Function to call whenever a line of output is
                 available from the STDOUT or STDERR streams of the process.
                 The function must accept a single string parameter.  The string
                 does not include the final newline.
+        @param output_params: Parameters to send to output_func before the
+                output line.
         @param output_prefix: String to prepend to lines sent to output_func.
         """
         # Add a reader
@@ -807,7 +845,9 @@ class kvm_shell_session(kvm_expect):
     """
 
     def __init__(self, command=None, id=None, echo=False, linesep="\n",
-                 termination_func=None, output_func=None, output_prefix="",
+                 termination_func=None, termination_params=(),
+                 output_func=None, output_params=(),
+                 output_prefix="",
                  prompt=r"[\#\$]\s*$", status_test_command="echo $?"):
         """
         Initialize the class and run command as a child process.
@@ -823,10 +863,14 @@ class kvm_shell_session(kvm_expect):
                 child process by sendline().
         @param termination_func: Function to call when the process exits.  The
                 function must accept a single exit status parameter.
+        @param termination_params: Parameters to send to termination_func
+                before the exit status.
         @param output_func: Function to call whenever a line of output is
                 available from the STDOUT or STDERR streams of the process.
                 The function must accept a single string parameter.  The string
                 does not include the final newline.
+        @param output_params: Parameters to send to output_func before the
+                output line.
         @param output_prefix: String to prepend to lines sent to output_func.
         @param prompt: Regular expression describing the shell's prompt line.
         @param status_test_command: Command to be used for getting the last
Signature · 1 line
1.5.4.1
#9 lore raw

PATCH 9/12 KVM test: dynamically detect and cache MAC-IP pairs using tcpdump

Run tcpdump in the background at all times and listen for DHCP communication. Whenever a DHCP server assigns an IP address to a MAC address, cache it for future reference.

---
 client/tests/kvm/kvm_preprocessing.py |   41 +++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)
client/tests/kvm/kvm_preprocessing.py +41 -0
index 4d77630..04bdb59 100644
@@ -196,6 +196,26 @@ def preprocess(test, params, env):
     @param params: A dict containing all VM and image parameters.
     @param env: The environment (a dict-like object).
     """
+    # Start tcpdump if it isn't already running
+    if not env.has_key("address_cache"):
+        env["address_cache"] = {}
+    if env.has_key("tcpdump") and not env["tcpdump"].is_alive():
+        env["tcpdump"].close()
+        del env["tcpdump"]
+    if not env.has_key("tcpdump"):
+        command = "/usr/sbin/tcpdump -npvi any 'dst port 68'"
+        logging.debug("Starting tcpdump (%s)...", command)
+        env["tcpdump"] = kvm_subprocess.kvm_tail(
+            command=command,
+            output_func=_update_address_cache,
+            output_params=(env["address_cache"],))
+        if kvm_utils.wait_for(lambda: not env["tcpdump"].is_alive(),
+                              0.1, 0.1, 1.0):
+            logging.warn("Could not start tcpdump")
+            logging.warn("Status: %s" % env["tcpdump"].get_status())
+            logging.warn("Output:" + kvm_utils.format_str_for_message(
+                env["tcpdump"].get_output()))
+
     # Destroy and remove VMs that are no longer needed in the environment
     requested_vms = kvm_utils.get_sub_dict_names(params, "vms")
     for key in env.keys():
@@ -280,6 +300,12 @@ def postprocess(test, params, env):
                         int(params.get("post_command_timeout", "600")),
                         params.get("post_command_noncritical") == "yes")
 
+    # Terminate tcpdump if no VMs are alive
+    living_vms = [vm for vm in kvm_utils.env_get_all_vms(env) if vm.is_alive()]
+    if not living_vms and env.has_key("tcpdump"):
+        env["tcpdump"].close()
+        del env["tcpdump"]
+
 
 def postprocess_on_error(test, params, env):
     """
@@ -290,3 +316,18 @@ def postprocess_on_error(test, params, env):
     @param env: The environment (a dict-like object).
     """
     params.update(kvm_utils.get_sub_dict(params, "on_error"))
+
+
+def _update_address_cache(address_cache, line):
+    if re.search("Your.IP", line, re.IGNORECASE):
+        matches = re.findall(r"\d*\.\d*\.\d*\.\d*", line)
+        if matches:
+            address_cache["last_seen"] = matches[0]
+    if re.search("Client.Ethernet.Address", line, re.IGNORECASE):
+        matches = re.findall(r"\w*:\w*:\w*:\w*:\w*:\w*", line)
+        if matches and address_cache.get("last_seen"):
+            mac_address = matches[0].lower()
+            logging.debug("(address cache) Adding cache entry: %s ---> %s",
+                          mac_address, address_cache.get("last_seen"))
+            address_cache[mac_address] = address_cache.get("last_seen")
+            del address_cache["last_seen"]
Signature · 1 line
1.5.4.1
#10 lore raw

PATCH 10/12 KVM test: kvm_utils.py: small fix for format_str_for_message()

format_str_for_message() sometimes adds an extra newline. Fix that.

---
 client/tests/kvm/kvm_utils.py |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
client/tests/kvm/kvm_utils.py +3 -1
index 5d8291f..1c11fa4 100644
@@ -654,7 +654,9 @@ def format_str_for_message(str):
 
     @param str: string that will be formatted.
     """
-    num_lines = len(str.splitlines())
+    lines = str.splitlines()
+    num_lines = len(lines)
+    str = "\n".join(lines)
     if num_lines == 0:
         return ""
     elif num_lines == 1:
Signature · 1 line
1.5.4.1
#11 lore raw

PATCH 11/12 KVM test: make VMs use a dynamic MAC-IP mapping generated by tcpdump

In VM.get_address(), return an IP address from the MAC-IP cache if an IP address base wasn't specified by the user, or if the user requested that IP addresses be taken from the dynamic cache.

To force the system to obtain IP addresses from the dynamic cache even when static base addresses are provided by the user, specify 'always_use_tcpdump = yes'.

---
 client/tests/kvm/kvm_address_pools.cfg.sample |    8 +++
 client/tests/kvm/kvm_preprocessing.py         |    3 +-
 client/tests/kvm/kvm_utils.py                 |   28 +++++++++++-
 client/tests/kvm/kvm_vm.py                    |   62 +++++++++++++++++-------
 4 files changed, 81 insertions(+), 20 deletions(-)
client/tests/kvm/kvm_address_pools.cfg.sample +8 -0
index 8a27ee1..debfe56 100644
@@ -6,6 +6,14 @@
 # If you wish to use a static MAC-IP mapping, where each MAC address range is
 # mapped to a known corresponding IP address range, specify the bases of the IP
 # address ranges in this file.
+# If you specify a MAC address range without a corresponding IP address range,
+# the IP addresses for that range will be determined at runtime by listening
+# to DHCP traffic using tcpdump.
+# If you wish to determine IP addresses using tcpdump in any case, regardless
+# of any # IP addresses specified in this file, uncomment the following line:
+#always_use_tcpdump = yes
+# You may also specify this parameter for specific hosts by adding it in the
+# appropriate sections below.
 
 variants:
     # Rename host1 to an actual (short) hostname in the network that will be running the Autotest client
client/tests/kvm/kvm_preprocessing.py +2 -1
index 04bdb59..a5a32dc 100644
@@ -54,7 +54,8 @@ def preprocess_vm(test, params, env, name):
         logging.debug("VM object found in environment")
     else:
         logging.debug("VM object does not exist; creating it")
-        vm = kvm_vm.VM(name, params, qemu_path, image_dir, iso_dir, script_dir)
+        vm = kvm_vm.VM(name, params, qemu_path, image_dir, iso_dir, script_dir,
+                       env.get("address_cache"))
         kvm_utils.env_register_vm(env, name, vm)
 
     start_vm = False
client/tests/kvm/kvm_utils.py +27 -1
index 1c11fa4..e17d84f 100644
@@ -1,5 +1,5 @@
 import md5, thread, subprocess, time, string, random, socket, os, signal, pty
-import select, re, logging
+import select, re, logging, commands
 #from autotest_lib.client.bin import utils
 #from autotest_lib.client.common_lib import error
 import kvm_subprocess
@@ -152,6 +152,32 @@ def get_mac_ip_pair_from_dict(dict):
     return (None, None)
 
 
+def verify_mac_ip_pair(mac, ip, timeout=3.0):
+    """
+    Connect to a given IP address and make sure its MAC address equals the
+    given MAC address.
+
+    @param mac: A MAC address.
+    @param ip: An IP address.
+    @return: True iff ip is assigned to mac.
+    """
+    s = socket.socket()
+    s.setblocking(False)
+    try:
+        s.connect((ip, 55555))
+    except socket.error:
+        pass
+    end_time = time.time() + timeout
+    while time.time() < end_time:
+        o = commands.getoutput("/sbin/arp -n")
+        if re.search(r"\b%s\b.*\b%s\b" % (ip, mac), o, re.IGNORECASE):
+            s.close()
+            return True
+        time.sleep(0.1)
+    s.close()
+    return False
+
+
 # Functions for working with the environment (a dict-like object)
 
 def is_vm(obj):
client/tests/kvm/kvm_vm.py +44 -18

173 lines not on this page · raw message

Signature · 1 line
1.5.4.1
#12 lore raw

PATCH 12/12 KVM test: make stress_boot work properly with TAP networking

Take an additional parameter 'clone_address_index_base' which indicates the initial value for 'address_index' for the cloned VMs. This value is incremented after each clone is created. I assume the original VM has a single NIC; otherwise NICs will end up sharing MAC addresses, which is bad.

Also, make a few small corrections:

- Take the params for the clones from the original VM's params, not from the test's params, because the test's params contain information about several VMs, only one of which is the original VM. The original VM's params, on the other hand, describe just a single VM (which we want to clone).

- Change the way kill_vm.* parameters are sent to the postprocessor. (The postprocessor doesn't read params from the VM objects but rather from the test's params dict.)

- Replace 'if get_command_status(...)' with 'if get_command_status(...) != 0'.

- Replace the test command 'ps aux' with 'uname -a'. The silly reason for this is that DSL-4.2.5 doesn't like 'ps aux'. Since 'uname -a' is just as good (AFAIK), use it instead.

---
 client/tests/kvm/kvm_tests.cfg.sample |    7 ++++++-
 client/tests/kvm/kvm_tests.py         |   14 ++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)
client/tests/kvm/kvm_tests.cfg.sample +6 -1
index 3c23432..3a4bf64 100644
@@ -117,7 +117,12 @@ variants:
     - stress_boot:  install setup
         type = stress_boot
         max_vms = 5    
-        alive_test_cmd = ps aux
+        alive_test_cmd = uname -a
+        clone_address_index_base = 10
+        kill_vm = yes
+        kill_vm_vm1 = no
+        kill_vm_gracefully = no
+        extra_params += " -snapshot"
 
     - shutdown:     install setup
         type = shutdown
client/tests/kvm/kvm_tests.py +6 -8
index 9784ec9..308db97 100644
@@ -541,8 +541,8 @@ def run_stress_boot(tests, params, env):
         raise error.TestFail("Could not log into first guest")
 
     num = 2
-    vms = []
     sessions = [session]
+    address_index = int(params.get("clone_address_index_base", 10))
 
     # boot the VMs
     while num <= int(params.get("max_vms")):
@@ -550,15 +550,12 @@ def run_stress_boot(tests, params, env):
             vm_name = "vm" + str(num)
 
             # clone vm according to the first one
-            vm_params = params.copy()
-            vm_params['image_snapshot'] = "yes"
-            vm_params['kill_vm'] = "yes"
-            vm_params['kill_vm_gracefully'] = "no"
+            vm_params = vm.get_params().copy()
+            vm_params["address_index"] = str(address_index)
             curr_vm = vm.clone(vm_name, vm_params)
             kvm_utils.env_register_vm(env, vm_name, curr_vm)
             params['vms'] += " " + vm_name
 
-            #vms.append(curr_vm)
             logging.info("Booting guest #%d" % num)
             if not curr_vm.create():
                 raise error.TestFail("Cannot create VM #%d" % num)
@@ -571,10 +568,11 @@ def run_stress_boot(tests, params, env):
             sessions.append(curr_vm_session)
 
             # check whether all previous ssh sessions are responsive
-            for i, vm_session in enumerate(sessions):
-                if vm_session.get_command_status(params.get("alive_test_cmd")):
+            for i, se in enumerate(sessions):
+                if se.get_command_status(params.get("alive_test_cmd")) != 0:
                     raise error.TestFail("Session #%d is not responsive" % i)
             num += 1
+            address_index += 1
 
         except (error.TestFail, OSError):
             for se in sessions:
Signature · 1 line
1.5.4.1
#13 lore raw

Re: [KVM-AUTOTEST PATCH 12/12] KVM test: make stress_boot work properly with TAP networking

Hi Michael, I just have some comments on what you changed on stress_boot. :-)

Quoted 33 lines · Michael Goldish

Take an additional parameter 'clone_address_index_base' which indicates the initial value for 'address_index' for the cloned VMs. This value is incremented after each clone is created. I assume the original VM has a single NIC; otherwise NICs will end up sharing MAC addresses, which is bad.

Also, make a few small corrections:

- Take the params for the clones from the original VM's params, not from the test's params, because the test's params contain information about several VMs, only one of which is the original VM. The original VM's params, on the other hand, describe just a single VM (which we want to clone).

- Change the way kill_vm.* parameters are sent to the postprocessor. (The postprocessor doesn't read params from the VM objects but rather from the test's params dict.)

- Replace 'if get_command_status(...)' with 'if get_command_status(...) != 0'.

- Replace the test command 'ps aux' with 'uname -a'. The silly reason for this is that DSL-4.2.5 doesn't like 'ps aux'. Since 'uname -a' is just as good (AFAIK), use it instead.

---
 client/tests/kvm/kvm_tests.cfg.sample |    7 ++++++-
 client/tests/kvm/kvm_tests.py         |   14 ++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)
client/tests/kvm/kvm_tests.cfg.sample +0 -0
index 3c23432..3a4bf64 100644

client/tests/kvm/kvm_tests.cfg.sample variants: · line 123

Quoted 8 lines
client/tests/kvm/kvm_tests.cfg.sample +4 -1
@@ -117,7 +117,12 @@ variants:
     - stress_boot:  install setup
         type = stress_boot
         max_vms = 5    
-        alive_test_cmd = ps aux
+        alive_test_cmd = uname -a
+        clone_address_index_base = 10
+        kill_vm = yes
+        kill_vm_vm1 = no

Actually, there are two methods to deal with started VMs via framework, one is as soon as the current test finished and the other is just before next test going to be started. In this case both methods equal somewhat between each other I think, and
I had chosen the later one. But if you had done some testing and
proved the previous one is better, I would agree.

client/tests/kvm/kvm_tests.cfg.sample variants: · line 124

Quoted 1 line
client/tests/kvm/kvm_tests.cfg.sample +1 -0
+        kill_vm_gracefully = no

This has been specified within code during cloning.

client/tests/kvm/kvm_tests.cfg.sample variants: · line 125

Quoted 1 line
client/tests/kvm/kvm_tests.cfg.sample +1 -0
+        extra_params += " -snapshot"

If you add this option '-snapshot' here, the first VM will be in snapshot mode as well which will affect next test.

Quoted 56 lines
client/tests/kvm/kvm_tests.cfg.sample +0 -0
     - shutdown:     install setup
         type = shutdown
client/tests/kvm/kvm_tests.py +6 -8
index 9784ec9..308db97 100644
@@ -541,8 +541,8 @@ def run_stress_boot(tests, params, env):
         raise error.TestFail("Could not log into first guest")
 
     num = 2
-    vms = []
     sessions = [session]
+    address_index = int(params.get("clone_address_index_base", 10))
 
     # boot the VMs
     while num <= int(params.get("max_vms")):
@@ -550,15 +550,12 @@ def run_stress_boot(tests, params, env):
             vm_name = "vm" + str(num)
 
             # clone vm according to the first one
-            vm_params = params.copy()
-            vm_params['image_snapshot'] = "yes"
-            vm_params['kill_vm'] = "yes"
-            vm_params['kill_vm_gracefully'] = "no"
+            vm_params = vm.get_params().copy()
+            vm_params["address_index"] = str(address_index)
             curr_vm = vm.clone(vm_name, vm_params)
             kvm_utils.env_register_vm(env, vm_name, curr_vm)
             params['vms'] += " " + vm_name
 
-            #vms.append(curr_vm)
             logging.info("Booting guest #%d" % num)
             if not curr_vm.create():
                 raise error.TestFail("Cannot create VM #%d" % num)
@@ -571,10 +568,11 @@ def run_stress_boot(tests, params, env):
             sessions.append(curr_vm_session)
 
             # check whether all previous ssh sessions are responsive
-            for i, vm_session in enumerate(sessions):
-                if vm_session.get_command_status(params.get("alive_test_cmd")):
+            for i, se in enumerate(sessions):
+                if se.get_command_status(params.get("alive_test_cmd")) != 0:
                     raise error.TestFail("Session #%d is not responsive" % i)
             num += 1
+            address_index += 1
 
         except (error.TestFail, OSError):
             for se in sessions:
Signature · 6 lines
1.5.4.1

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#14 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 07/12] KVM test: kvm_config.py: do not fail when including a nonexistent file

In reply to Michael Goldish
Quoted 25 lines · Michael Goldish

Instead of failing, just print a warning.  Included files may not always be crucial for tests to run (kvm_cdkeys.cfg for example).

---
 client/tests/kvm/kvm_config.py |   21 +++++++++++----------

 1 files changed, 11 insertions(+), 10 deletions(-)
client/tests/kvm/kvm_config.py +0 -0
index e478a55..7ff7a07 100755
@@ -406,17 +406,18 @@ class config:

                if self.filename:
                    filename = os.path.join(os.path.dirname(self.filename), words[1])
-                    if not os.path.exists(filename):
-                        e_msg = "Cannot include %s -- file not found" % filename
-                        raise error.AutotestError(e_msg)
-                    new_file = open(filename, "r")
-                    list = self.parse(new_file, list, restricted)
-                    new_file.close()
-                    if self.debug and not restricted:
-                        self.__debug_print("", "Leaving file %s" % words[1])
+                    if os.path.exists(filename):

Quoted 8 lines

+                        new_file = open(filename, "r")
+                        list = self.parse(new_file, list, restricted)
+                        new_file.close()
+                        if self.debug and not restricted:
+                            self.__debug_print("", "Leaving file %s" % words[1])
+                    else:
+                        print ("WARNING: Cannot include %s -- "
+                               "file not found" % filename)

I quite didn't like the above, because to issue messages like that we should be using the logging infrastructure to avoid to write WARNING, DEBUG and similar strings. Anyway, I understand why kvm_config.py is still using print's. I will apply this, but will send later a patch that makes kvm_config.py to use the logging infrastructure.

#15 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 07/12] KVM test: kvm_config.py: do not fail when including a nonexistent file

Quoted 43 lines · ----- "Lucas Meneghel Rodrigues
Quoted 1 line · Michael Goldish

Instead of failing, just print a warning.  Included files may not

always be

Review

Quoted 8 lines

crucial for tests to run (kvm_cdkeys.cfg for example).

---
 client/tests/kvm/kvm_config.py |   21 +++++++++++----------

 1 files changed, 11 insertions(+), 10 deletions(-)
(quoted diff) +0 -0

b/client/tests/kvm/kvm_config.py

Quoted 6 lines

index e478a55..7ff7a07 100755

client/tests/kvm/kvm_config.py +0 -0
@@ -406,17 +406,18 @@ class config:

                if self.filename:
                    filename =

os.path.join(os.path.dirname(self.filename),

Quoted 3 lines

                                            words[1])
-                    if not os.path.exists(filename):
-                        e_msg = "Cannot include %s -- file not

found" % filename

Quoted 6 lines

-                        raise error.AutotestError(e_msg)
-                    new_file = open(filename, "r")
-                    list = self.parse(new_file, list, restricted)
-                    new_file.close()
-                    if self.debug and not restricted:
-                        self.__debug_print("", "Leaving file %s" %

words[1])

Quoted 3 lines

+                    if os.path.exists(filename):
+                        new_file = open(filename, "r")
+                        list = self.parse(new_file, list,

restricted)

Quoted 3 lines

+                        new_file.close()
+                        if self.debug and not restricted:
+                            self.__debug_print("", "Leaving file

%s" % words[1])

Quoted 3 lines

+                    else:
+                        print ("WARNING: Cannot include %s -- "
+                               "file not found" % filename)

Quoted 5 lines

I quite didn't like the above, because to issue messages like that we should be using the logging infrastructure to avoid to write WARNING, DEBUG and similar strings. Anyway, I understand why kvm_config.py is still using print's. I will apply this, but will send later a patch that makes kvm_config.py to use the logging infrastructure.

Can we use the logging infrastructure in stand-alone modules?

#16 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 07/12] KVM test: kvm_config.py: do not fail when including a nonexistent file

Quoted 9 lines · Michael Goldish
Quoted 5 lines · ----- "Lucas Meneghel Rodrigues
Quoted 1 line

I quite didn't like the above, because to issue messages like that we

should be using the logging infrastructure to avoid to write WARNING, DEBUG and similar strings. Anyway, I understand why kvm_config.py is still using print's. I will apply this, but will send later a patch that makes kvm_config.py to use the logging infrastructure.

Can we use the logging infrastructure in stand-alone modules?

Yes, we can, we have logging_manager and logging_config have what is needed. I am going to send a patch showing how to do it with kvm_config.py.

#17 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 06/12] KVM test: minor style changes to kvm_config.py

In reply to Michael Goldish
Quoted 4 lines · Michael Goldish

- Fix indentation of a few lines.
- Limit lines to 80 characters.
- Use list comprehension in config.filter().
- Replace '== None' with 'is None'.

Good, applied!

Quoted 100 lines

---
 client/tests/kvm/kvm_config.py |   34 ++++++++++++++++++----------------

 1 files changed, 18 insertions(+), 16 deletions(-)
client/tests/kvm/kvm_config.py +0 -0
index 7e8b1db..e478a55 100755
@@ -83,8 +83,8 @@ class config:

        @param filter: A regular expression that defines the filter. @param dict: Dictionary that will be inspected. """
-        filter = re.compile("(\\.|^)" + filter + "(\\.|$)")
-        return filter.search(dict["name"]) != None
+        filter = re.compile(r"(\.|^)" + filter + r"(\.|$)")
+        return bool(filter.search(dict["name"]))

    def filter(self, filter, list=None):

client/tests/kvm/kvm_config.py +0 -0
@@ -94,13 +94,9 @@ class config:

        @param filter: A regular expression that will be used as a filter. @param list: A list of dictionaries that will be filtered. """
-        if list == None:
+        if list is None:
            list = self.list
-        filtered_list = []
-        for dict in list:
-            if self.match(filter, dict):
-                filtered_list.append(dict)
-        return filtered_list
+        return [dict for dict in list if self.match(filter, dict)]

    # Currently unused, will be removed if it remains unused

client/tests/kvm/kvm_config.py +0 -0
@@ -111,7 +107,7 @@ class config:

        @param filter: A regular expression that will filter the list. @param list: List which we want to know the indexes that match a filter. """
-        if list == None:
+        if list is None:
            list = self.list
        block_list = []
        prev_match = False

client/tests/kvm/kvm_config.py +0 -0
@@ -248,17 +244,22 @@ class config:
                 temp_list.append(new_dict)
 

            if subvariants:
-                # If we're parsing 'subvariants', we need to modify the list first
-                self.__modify_list_subvariants(temp_list, name, dep_list, add_to_shortname)
+                # If we're parsing 'subvariants', first modify the list
+                self.__modify_list_subvariants(temp_list, name, dep_list,
+                                               add_to_shortname)
                temp_list = self.parse(file, temp_list,
                        restricted=True, prev_indent=indent)
            else:
-                # If we're parsing 'variants', we need to parse first and then modify the list
+                # If we're parsing 'variants', parse before modifying the list
                if self.debug:
-                    self.__debug_print(indented_line, "Entering variant '%s' (variant inherits %d dicts)" % (name, len(list)))
+                    self.__debug_print(indented_line,
+                                       "Entering variant '%s' "
+                                       "(variant inherits %d dicts)" %
+                                       (name, len(list)))
                temp_list = self.parse(file, temp_list,
                        restricted=False, prev_indent=indent)
-                self.__modify_list_variants(temp_list, name, dep_list, add_to_shortname)
+                self.__modify_list_variants(temp_list, name, dep_list,
+                                            add_to_shortname)

            new_list += temp_list

client/tests/kvm/kvm_config.py +0 -0
@@ -401,7 +402,7 @@ class config:

                    continue
                if self.debug and not restricted:
                    self.__debug_print(indented_line,
-                                     "Entering file %s" % words[1])
+                                       "Entering file %s" % words[1]) if self.filename:
                    filename = os.path.join(os.path.dirname(self.filename), words[1])

client/tests/kvm/kvm_config.py +2 -1
@@ -440,7 +441,8 @@ class config:
                         # Everything inside an exception should be parsed in
                         # restricted mode
                         new_list += self.parse(file, list[index:index+1],
-                                restricted=True, prev_indent=indent)
+                                               restricted=True,
+                                               prev_indent=indent)

                    else:
                        new_list += list[index:index+1] list = new_list
--
1.5.4.1

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Signature · 1 line
Lucas Meneghel
#18 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 05/12] KVM test: specify installation cdkeys in a dedicated file

In reply to Michael Goldish
Quoted 6 lines · Michael Goldish

- Add file kvm_cdkeys.cfg.sample which specifies cdkeys for guests that require them.  Users should rename the file to kvm_cdkeys.cfg and enter the real cdkeys in the file.

- Remove all cdkey lines from kvm_tests.cfg.sample and add an include statement instead ('include kvm_cdkeys.cfg').

Talking about this one, I like the idea of separating the cd keys, and it's my understanding the configuration snippets can be moved inside the control file so one can more comfortably add it from the web interface in the future. Also, the unattended install test will implement answer file templating, which will allow for comfortable replacement of those keys once without requiring user intervention.

Quoted 106 lines

---
 client/tests/kvm/kvm_cdkeys.cfg.sample |   16 ++++++++++++++++ client/tests/kvm/kvm_tests.cfg.sample  |   10 +++-------

 2 files changed, 19 insertions(+), 7 deletions(-)

 create mode 100644 client/tests/kvm/kvm_cdkeys.cfg.sample

client/tests/kvm/kvm_cdkeys.cfg.sample new file +16 -0
new file mode 100644
index 0000000..60bfe59
@@ -0,0 +1,16 @@
+# Rename this file to kvm_cdkeys.cfg.
+# Replace the 'CDKEY' strings with real cdkeys where necessary.
+# Guests that do not require cdkeys by default are commented out.
+# You may uncomment them if necessary.
+
+RHEL.5.3.i386: cdkey = CDKEY
+RHEL.5.3.x86_64: cdkey = CDKEY
+Win2000: cdkey = CDKEY
+WinXP.32: cdkey = CDKEY
+WinXP.64: cdkey = CDKEY
+Win2003.32: cdkey = CDKEY
+Win2003.64: cdkey = CDKEY
+#WinVista.32: cdkey = CDKEY
+#WinVista.64: cdkey = CDKEY
+#Win2008.32: cdkey = CDKEY
+#Win2008.64: cdkey = CDKEY
client/tests/kvm/kvm_tests.cfg.sample +0 -0
index 4b9d7eb..3c23432 100644
@@ -272,7 +272,6 @@ variants:

                        image_name = rhel5-32
                        install:
                            steps=RHEL-5.3-i386.steps
-                            cdkey=RHEL_INSTALL_NUMBER cdrom=linux/RHEL-5.3-i386-DVD.iso md5sum=371c62851611fd32ead440df6f24a296 md5sum_1m=242318dd44152210f6ff6cdda1bfbf51

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -282,7 +281,6 @@ variants:

                        image_name = rhel5-64
                        install:
                            steps=RHEL-5.3-x86_64.steps
-                            cdkey=RHEL_INSTALL_NUMBER cdrom=linux/RHEL-5.3-x86_64-DVD.iso md5sum=c5ed6b284410f4d8212cafc78fd7a8c5 md5sum_1m=b999f437583098ea5bbd56fb1de1d011

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -363,7 +361,6 @@ variants:

                    md5sum = dda6039f3a9173f0f6bfae40f5efdfea md5sum_1m = dd28fba196d366d56fe774bd93df5527 user = user
-                    cdkey = WIN_2000_32_CDKEY
                setup:
                    steps = Win2000-32-setupssh.steps cdrom = windows/setupssh.iso

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -376,7 +373,6 @@ variants:

                    md5sum = 743450644b1d9fe97b3cf379e22dceb0 md5sum_1m = b473bf75af2d1269fec8958cf0202bfd user = user
-                    cdkey = WIN_XP_32_CDKEY
                setup:
                    steps = WinXP-32-setupssh.steps cdrom = windows/setupssh.iso

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -390,7 +386,6 @@ variants:

                    md5sum = 8d3f007ec9c2060cec8a50ee7d7dc512 md5sum_1m = e812363ff427effc512b7801ee70e513 user = user
-                    cdkey = WIN_XP_64_CDKEY

            - Win2003:
                image_size = 20G

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -406,7 +401,6 @@ variants:

                            md5sum = 03e921e9b4214773c21a39f5c3f42ef7 md5sum_1m = 37c2fdec15ac4ec16aa10fdfdb338aa3 user = user
-                            cdkey = WIN_2003_32_CDKEY setup:
                            steps = Win2003-32-setupssh.steps cdrom = windows/setupssh.iso

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -420,7 +414,6 @@ variants:

                            md5sum = 5703f87c9fd77d28c05ffadd3354dbbd md5sum_1m = 439393c384116aa09e08a0ad047dcea8 user = user
-                            cdkey = WIN_2003_64_CDKEY

            - WinVista:
                only install

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -624,5 +617,8 @@ variants:

        only rtl8139

client/tests/kvm/kvm_tests.cfg.sample +3 -0
+include kvm_cdkeys.cfg
+
+

 # Choose your test list
 only fc8_quick
--
1.5.4.1

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Signature · 1 line
Lucas Meneghel
#19 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 05/12] KVM test: specify installation cdkeys in a dedicated file

Quoted 10 lines · ----- "Lucas Meneghel Rodrigues
Quoted 1 line · Michael Goldish

- Add file kvm_cdkeys.cfg.sample which specifies cdkeys for guests

that require

Quoted 1 line

 them.  Users should rename the file to kvm_cdkeys.cfg and enter the

real

Quoted 3 lines

 cdkeys in the file.

- Remove all cdkey lines from kvm_tests.cfg.sample and add an

include statement

Quoted 8 lines
Quoted 1 line

 instead ('include kvm_cdkeys.cfg').

Talking about this one, I like the idea of separating the cd keys, and it's my understanding the configuration snippets can be moved inside the control file so one can more comfortably add it from the web interface in the future. Also, the unattended install test will implement answer file templating, which will allow for comfortable replacement of those keys once without requiring user intervention.

Does that mean you like the patch? Or do you need more time to review it?

The ability to modify the configuration in the control file isn't very relevant to the cdkeys because we probably don't want to specify cdkeys in the control file.

Quoted 119 lines

Review

Quoted 8 lines

---
 client/tests/kvm/kvm_cdkeys.cfg.sample |   16 ++++++++++++++++ client/tests/kvm/kvm_tests.cfg.sample  |   10 +++-------

 2 files changed, 19 insertions(+), 7 deletions(-)

 create mode 100644 client/tests/kvm/kvm_cdkeys.cfg.sample

(quoted diff) +0 -0

b/client/tests/kvm/kvm_cdkeys.cfg.sample

Review

Quoted 22 lines

new file mode 100644
index 0000000..60bfe59

client/tests/kvm/kvm_cdkeys.cfg.sample new file +16 -0
@@ -0,0 +1,16 @@
+# Rename this file to kvm_cdkeys.cfg.
+# Replace the 'CDKEY' strings with real cdkeys where necessary.
+# Guests that do not require cdkeys by default are commented out.
+# You may uncomment them if necessary.
+
+RHEL.5.3.i386: cdkey = CDKEY
+RHEL.5.3.x86_64: cdkey = CDKEY
+Win2000: cdkey = CDKEY
+WinXP.32: cdkey = CDKEY
+WinXP.64: cdkey = CDKEY
+Win2003.32: cdkey = CDKEY
+Win2003.64: cdkey = CDKEY
+#WinVista.32: cdkey = CDKEY
+#WinVista.64: cdkey = CDKEY
+#Win2008.32: cdkey = CDKEY
+#Win2008.64: cdkey = CDKEY
(quoted diff) +0 -0

b/client/tests/kvm/kvm_tests.cfg.sample

Quoted 11 lines

index 4b9d7eb..3c23432 100644

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -272,7 +272,6 @@ variants:

                        image_name = rhel5-32
                        install:
                            steps=RHEL-5.3-i386.steps
-                            cdkey=RHEL_INSTALL_NUMBER cdrom=linux/RHEL-5.3-i386-DVD.iso md5sum=371c62851611fd32ead440df6f24a296

md5sum_1m=242318dd44152210f6ff6cdda1bfbf51

Quoted 8 lines
client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -282,7 +281,6 @@ variants:

                        image_name = rhel5-64
                        install:
                            steps=RHEL-5.3-x86_64.steps
-                            cdkey=RHEL_INSTALL_NUMBER cdrom=linux/RHEL-5.3-x86_64-DVD.iso md5sum=c5ed6b284410f4d8212cafc78fd7a8c5

md5sum_1m=b999f437583098ea5bbd56fb1de1d011

Quoted 25 lines
client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -363,7 +361,6 @@ variants:

                    md5sum = dda6039f3a9173f0f6bfae40f5efdfea md5sum_1m = dd28fba196d366d56fe774bd93df5527 user = user
-                    cdkey = WIN_2000_32_CDKEY
                setup:
                    steps = Win2000-32-setupssh.steps cdrom = windows/setupssh.iso

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -376,7 +373,6 @@ variants:

                    md5sum = 743450644b1d9fe97b3cf379e22dceb0 md5sum_1m = b473bf75af2d1269fec8958cf0202bfd user = user
-                    cdkey = WIN_XP_32_CDKEY
                setup:
                    steps = WinXP-32-setupssh.steps cdrom = windows/setupssh.iso

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -390,7 +386,6 @@ variants:

                    md5sum = 8d3f007ec9c2060cec8a50ee7d7dc512 md5sum_1m = e812363ff427effc512b7801ee70e513 user = user
-                    cdkey = WIN_XP_64_CDKEY

            - Win2003:
                image_size = 20G

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -406,7 +401,6 @@ variants:
Quoted 1 line

                            md5sum =

03e921e9b4214773c21a39f5c3f42ef7

Quoted 1 line

                            md5sum_1m =

37c2fdec15ac4ec16aa10fdfdb338aa3

Quoted 7 lines

                            user = user
-                            cdkey = WIN_2003_32_CDKEY setup:
                            steps = Win2003-32-setupssh.steps cdrom = windows/setupssh.iso

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -420,7 +414,6 @@ variants:

                            md5sum =

5703f87c9fd77d28c05ffadd3354dbbd

Quoted 1 line

                            md5sum_1m =

439393c384116aa09e08a0ad047dcea8

Quoted 22 lines

                            user = user
-                            cdkey = WIN_2003_64_CDKEY

            - WinVista:
                only install

client/tests/kvm/kvm_tests.cfg.sample +0 -0
@@ -624,5 +617,8 @@ variants:

        only rtl8139

client/tests/kvm/kvm_tests.cfg.sample +3 -0
+include kvm_cdkeys.cfg
+
+

 # Choose your test list
 only fc8_quick
--
1.5.4.1

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Signature · 1 line
Lucas Meneghel
#20 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 03/12] KVM test: add sample 'qemu-ifup' script

In reply to Michael Goldish

I am taking some time to review your patches, and likewise you mentioned revising my unattended patchset, it's going to take sometime for me to go trough all the code. Starting with the low hanging fruit, this little setup script could be turned into a python script as well!

Quoted 36 lines · Michael Goldish

The script adds a requested interface to an existing bridge.  It is meant to be used by qemu when running in TAP mode.

Note: the user is responsible for setting up the bridge before running any tests.  This can be done with brctl or in any manner that is appropriate for the host OS.  It can be done inside 'qemu-ifup' as well, but this sample script doesn't do it.

---
 client/tests/kvm/qemu-ifup |    8 ++++++++

 1 files changed, 8 insertions(+), 0 deletions(-)

 create mode 100644 client/tests/kvm/qemu-ifup

client/tests/kvm/qemu-ifup new file +8 -1
new file mode 100644
index 0000000..bcd9a7a
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+# The following expression selects the first bridge listed by 'brctl show'.
+# Modify it to suit your needs.
+switch=$(/usr/sbin/brctl show | awk 'NR==2 { print $1 }')
+
+/sbin/ifconfig $1 0.0.0.0 up
+/usr/sbin/brctl addif ${switch} $1
--

1.5.4.1

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Signature · 1 line
Lucas Meneghel
#21 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 03/12] KVM test: add sample 'qemu-ifup' script

Quoted 7 lines · ----- "Lucas Meneghel Rodrigues

I am taking some time to review your patches, and likewise you mentioned revising my unattended patchset, it's going to take sometime
for me to go trough all the code. Starting with the low hanging fruit,
this little setup script could be turned into a python script as well!

qemu-ifup is a traditional qemu script. The one in this patch is almost identical to the ones included in KVM releases. Also, it's meant to be modified by the user -- the user may want to replace the 'brctl show | awk' expression with the name of a bridge, especially if the host has more than one. I think a python script will be awkward to modify.
Also, traditionally this script resides in /etc, and this one is provided only in case the user doesn't have a better one in /etc. The script in /etc is normally a bash script.

I have no problem with rewriting this as a python script -- I just think it's more natural to keep this one in bash.
In python it would look something like:

import sys, os, commands
switch = commands.getoutput("/usr/sbin/brctl show").split()[1].split()[0] os.system("/sbin/ifconfig %s 0.0.0.0 up" % sys.argv[1]) os.system("/usr/sbin/brctl addif %s %s" % (switch, sys.argv[1]))

There's nothing 'pythonic' about this. It looks like it should be a bash script. It also looks simpler in bash. Anyway, if you like this better, or if you think the 'python only' policy should apply here, no problem.

Quoted 53 lines
Quoted 1 line · Michael Goldish

The script adds a requested interface to an existing bridge.  It is

meant to be

Quoted 3 lines

used by qemu when running in TAP mode.

Note: the user is responsible for setting up the bridge before

running any

Quoted 1 line

tests.  This can be done with brctl or in any manner that is

appropriate for

Quoted 1 line

the host OS.  It can be done inside 'qemu-ifup' as well, but this

sample script

Review

Quoted 9 lines

doesn't do it.

---
 client/tests/kvm/qemu-ifup |    8 ++++++++

 1 files changed, 8 insertions(+), 0 deletions(-)

 create mode 100644 client/tests/kvm/qemu-ifup

(quoted diff) +0 -0

b/client/tests/kvm/qemu-ifup

client/tests/kvm/qemu-ifup line 3

Quoted 8 lines

new file mode 100644
index 0000000..bcd9a7a

client/tests/kvm/qemu-ifup new file +3 -0
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+# The following expression selects the first bridge listed by

'brctl show'.

Quoted 13 lines
client/tests/kvm/qemu-ifup new file +5 -1
+# Modify it to suit your needs.
+switch=$(/usr/sbin/brctl show | awk 'NR==2 { print $1 }')
+
+/sbin/ifconfig $1 0.0.0.0 up
+/usr/sbin/brctl addif ${switch} $1
--

1.5.4.1

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Signature · 5 lines
Lucas Meneghel
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#22 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 03/12] KVM test: add sample 'qemu-ifup' script

Lets not make it a python script. Since the purpose of providing this script is that the user can copy it to /etc also and not bother updating it to kvm_tests.cfg, so let us keep it bash only. Also as Michael pointed there is nothing much pythonic even if we write it in python, so better keep it bash.

Quoted 93 lines · Michael Goldish
Quoted 7 lines · ----- "Lucas Meneghel Rodrigues

I am taking some time to review your patches, and likewise you mentioned revising my unattended patchset, it's going to take sometime
for me to go trough all the code. Starting with the low hanging fruit,
this little setup script could be turned into a python script as well!

qemu-ifup is a traditional qemu script. The one in this patch is almost identical to the ones included in KVM releases. Also, it's meant to be modified by the user -- the user may want to replace the 'brctl show | awk' expression with the name of a bridge, especially if the host has more than one. I think a python script will be awkward to modify.
Also, traditionally this script resides in /etc, and this one is provided only in case the user doesn't have a better one in /etc. The script in /etc is normally a bash script.

I have no problem with rewriting this as a python script -- I just think it's more natural to keep this one in bash.
In python it would look something like:

import sys, os, commands
switch = commands.getoutput("/usr/sbin/brctl show").split()[1].split()[0] os.system("/sbin/ifconfig %s 0.0.0.0 up" % sys.argv[1]) os.system("/usr/sbin/brctl addif %s %s" % (switch, sys.argv[1]))

There's nothing 'pythonic' about this. It looks like it should be a bash script. It also looks simpler in bash. Anyway, if you like this better, or if you think the 'python only' policy should apply here, no problem.

Quoted 39 lines
Quoted 1 line · Michael Goldish

The script adds a requested interface to an existing bridge.  It is

meant to be

Quoted 3 lines

used by qemu when running in TAP mode.

Note: the user is responsible for setting up the bridge before

running any

Quoted 1 line

tests.  This can be done with brctl or in any manner that is

appropriate for

Quoted 1 line

the host OS.  It can be done inside 'qemu-ifup' as well, but this

sample script

Review

Quoted 9 lines

doesn't do it.

---
 client/tests/kvm/qemu-ifup |    8 ++++++++

 1 files changed, 8 insertions(+), 0 deletions(-)

 create mode 100644 client/tests/kvm/qemu-ifup

(quoted diff) +0 -0

b/client/tests/kvm/qemu-ifup

client/tests/kvm/qemu-ifup line 3

Quoted 8 lines

new file mode 100644
index 0000000..bcd9a7a

client/tests/kvm/qemu-ifup new file +3 -0
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+# The following expression selects the first bridge listed by

'brctl show'.

Quoted 8 lines
client/tests/kvm/qemu-ifup new file +5 -1
+# Modify it to suit your needs.
+switch=$(/usr/sbin/brctl show | awk 'NR==2 { print $1 }')
+
+/sbin/ifconfig $1 0.0.0.0 up
+/usr/sbin/brctl addif ${switch} $1
--

1.5.4.1

Quoted 14 lines
Quoted 5 lines

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

--
Lucas Meneghel
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Signature · 1 line
Sudhir Kumar
#23 lore raw

Re: [Autotest] [KVM-AUTOTEST PATCH 03/12] KVM test: add sample 'qemu-ifup' script

Quoted 5 lines · sudhir kumar

Lets not make it a python script. Since the purpose of providing this script is that the user can copy it to /etc also and not bother updating it to kvm_tests.cfg, so let us keep it bash only. Also as Michael pointed there is nothing much pythonic even if we write it in python, so better keep it bash.

Ok folks, fair enough :) Let's keep it bash, in this case I think it won't hurt. Thanks for your input!

Quoted 104 lines · Michael Goldish
Quoted 88 lines
Quoted 7 lines · ----- "Lucas Meneghel Rodrigues

I am taking some time to review your patches, and likewise you mentioned revising my unattended patchset, it's going to take sometime
for me to go trough all the code. Starting with the low hanging fruit,
this little setup script could be turned into a python script as well!

qemu-ifup is a traditional qemu script. The one in this patch is almost identical to the ones included in KVM releases. Also, it's meant to be modified by the user -- the user may want to replace the 'brctl show | awk' expression with the name of a bridge, especially if the host has more than one. I think a python script will be awkward to modify.
Also, traditionally this script resides in /etc, and this one is provided only in case the user doesn't have a better one in /etc. The script in /etc is normally a bash script.

I have no problem with rewriting this as a python script -- I just think it's more natural to keep this one in bash.
In python it would look something like:

import sys, os, commands
switch = commands.getoutput("/usr/sbin/brctl show").split()[1].split()[0] os.system("/sbin/ifconfig %s 0.0.0.0 up" % sys.argv[1]) os.system("/usr/sbin/brctl addif %s %s" % (switch, sys.argv[1]))

There's nothing 'pythonic' about this. It looks like it should be a bash script. It also looks simpler in bash. Anyway, if you like this better, or if you think the 'python only' policy should apply here, no problem.

Quoted 39 lines
Quoted 1 line · Michael Goldish

The script adds a requested interface to an existing bridge.  It is

meant to be

Quoted 3 lines

used by qemu when running in TAP mode.

Note: the user is responsible for setting up the bridge before

running any

Quoted 1 line

tests.  This can be done with brctl or in any manner that is

appropriate for

Quoted 1 line

the host OS.  It can be done inside 'qemu-ifup' as well, but this

sample script

Review

Quoted 9 lines

doesn't do it.

---
 client/tests/kvm/qemu-ifup |    8 ++++++++

 1 files changed, 8 insertions(+), 0 deletions(-)

 create mode 100644 client/tests/kvm/qemu-ifup

(quoted diff) +0 -0

b/client/tests/kvm/qemu-ifup

client/tests/kvm/qemu-ifup line 3

Quoted 8 lines

new file mode 100644
index 0000000..bcd9a7a

client/tests/kvm/qemu-ifup new file +3 -0
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+# The following expression selects the first bridge listed by

'brctl show'.

Quoted 8 lines
client/tests/kvm/qemu-ifup new file +5 -1
+# Modify it to suit your needs.
+switch=$(/usr/sbin/brctl show | awk 'NR==2 { print $1 }')
+
+/sbin/ifconfig $1 0.0.0.0 up
+/usr/sbin/brctl addif ${switch} $1
--

1.5.4.1

Quoted 14 lines
Quoted 5 lines

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

--
Lucas Meneghel
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

--
Sudhir Kumar
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Signature · 1 line
Lucas Meneghel
#24 lore raw
In reply to Michael Goldish
Quoted 11 lines · Michael Goldish

Here's my proposed TAP solution:
- The user specifies MAC ranges and may or may not specify corresponding IP ranges.
- VMs are always given MAC addresses from the user specified MAC ranges.
- MAC address ranges must be unique to each host running KVM tests -- there must be no overlap between them.
- If corresponding IP ranges are specified as well, the system will use them when trying to communicate with guests.
- If corresponding IP ranges are not specified, tcpdump will be used to listen to DHCP traffic and dynamically detect the right IP addresses. There's also a flag that can force this behavior.

This is pretty nice patch set and it's nice and friendly to hook the assigned ip address. Maybe instead of using tcpdump you can add a specific ip tables rule to trap dhcp replies and log them?

Quoted 25 lines

- tcpdump will run in the background at all times, and collect MAC-IP pairs as soon as they are assigned by the DHCP server. This is useful for NIC hotplugging (where IP addresses are assigned during the test itself) and generally for misbehaving guests or DHCP servers (restarting network services during the test, using very short lease times).
- It is up to the user to create the actual bridge for TAP devices; the qemu-ifup script included in one of the patches only adds the TAP interface to an existing bridge. The user should modify this script or create a bridge some other way.

This works very well on two hosts I tried, but I'm not entirely sure it will work in all cases -- please let me know what you think.

(I remember Jason Wang mentioned that tcpdump doesn't always catch all the required traffic, or that it might somehow depend on the DHCP server -- anyone willing to comment on that?)

The following patches have been tested with a few KVM tests (boot, reboot, stress_boot, migration) with both TAP and user mode, but they could probably use more testing (maybe with Python 2.6?), so if anyone is willing to help with that I'd appreciate it very much.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#25 lore raw
Quoted 16 lines · ----- "Dor Laor
Quoted 2 lines · Michael Goldish

Here's my proposed TAP solution:
- The user specifies MAC ranges and may or may not specify

corresponding IP

Quoted 2 lines

ranges.
- VMs are always given MAC addresses from the user specified MAC

ranges.

Quoted 1 line

- MAC address ranges must be unique to each host running KVM tests

-- there

Quoted 2 lines

must be no overlap between them.
- If corresponding IP ranges are specified as well, the system will

use them

Quoted 2 lines

when trying to communicate with guests.
- If corresponding IP ranges are not specified, tcpdump will be used

to listen

Quoted 1 line

to DHCP traffic and dynamically detect the right IP addresses.

Quoted 8 lines

There's also a

Quoted 1 line

flag that can force this behavior.

This is pretty nice patch set and it's nice and friendly to hook the assigned ip address.
Maybe instead of using tcpdump you can add a specific ip tables rule to
trap dhcp replies and log them?

What are the advantages compared to tcpdump? Is it easier? If you look at the tcpdump patch you'll see it's quite short. Most of the code in this set is not related to tcpdump but rather to host specific static MAC-IP mapping and other related things. To me the iptables idea sounds a little more complex especially since we'll have to deal with host specific configuration -- I'm not even sure all hosts run a firewall. What do you think?

Quoted 38 lines
Quoted 1 line

- tcpdump will run in the background at all times, and collect

MAC-IP pairs

Quoted 1 line

as soon as they are assigned by the DHCP server. This is useful

for

Quoted 1 line

NIC hotplugging (where IP addresses are assigned during the test

itself) and

Quoted 1 line

generally for misbehaving guests or DHCP servers (restarting network

services

Quoted 2 lines

during the test, using very short lease times).
- It is up to the user to create the actual bridge for TAP devices;

the

Quoted 1 line

qemu-ifup script included in one of the patches only adds the TAP

interface to

Quoted 1 line

an existing bridge. The user should modify this script or create a

bridge

Quoted 3 lines

some other way.

This works very well on two hosts I tried, but I'm not entirely sure

it will

Quoted 3 lines

work in all cases -- please let me know what you think.

(I remember Jason Wang mentioned that tcpdump doesn't always catch

all the

Quoted 1 line

required traffic, or that it might somehow depend on the DHCP server

-- anyone

Quoted 3 lines

willing to comment on that?)

The following patches have been tested with a few KVM tests (boot,

reboot,

Quoted 1 line

stress_boot, migration) with both TAP and user mode, but they could

probably

Quoted 1 line

use more testing (maybe with Python 2.6?), so if anyone is willing

to help

Quoted 5 lines

with that I'd appreciate it very much.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#26 lore raw
Quoted 26 lines · Michael Goldish
Quoted 15 lines · ----- "Dor Laor
Quoted 2 lines · Michael Goldish

Here's my proposed TAP solution:
- The user specifies MAC ranges and may or may not specify

corresponding IP

Quoted 2 lines

ranges.
- VMs are always given MAC addresses from the user specified MAC

ranges.

Quoted 1 line

- MAC address ranges must be unique to each host running KVM tests

-- there

Quoted 2 lines

must be no overlap between them.
- If corresponding IP ranges are specified as well, the system will

use them

Quoted 2 lines

when trying to communicate with guests.
- If corresponding IP ranges are not specified, tcpdump will be used

to listen

Quoted 8 lines
Quoted 1 line

to DHCP traffic and dynamically detect the right IP addresses.

There's also a

Quoted 1 line

flag that can force this behavior.

This is pretty nice patch set and it's nice and friendly to hook the assigned ip address.
Maybe instead of using tcpdump you can add a specific ip tables rule to
trap dhcp replies and log them?

Quoted 7 lines

What are the advantages compared to tcpdump? Is it easier? If you look at the tcpdump patch you'll see it's quite short. Most of the code in this set is not related to tcpdump but rather to host specific static MAC-IP mapping and other related things. To me the iptables idea sounds a little more complex especially since we'll have to deal with host specific configuration -- I'm not even sure all hosts run a firewall. What do you think?

iptables config might be a bit harder but on the same scale..
If you plan to constantly run tcpdump in the background iptables hook will be much cheaper.

Quoted 43 lines
Quoted 26 lines
Quoted 2 lines

- tcpdump will run in the background at all times, and collect

MAC-IP pairs

Quoted 1 line

as soon as they are assigned by the DHCP server. This is useful

for

Quoted 1 line

NIC hotplugging (where IP addresses are assigned during the test

itself) and

Quoted 1 line

generally for misbehaving guests or DHCP servers (restarting network

services

Quoted 2 lines

during the test, using very short lease times).
- It is up to the user to create the actual bridge for TAP devices;

the

Quoted 1 line

qemu-ifup script included in one of the patches only adds the TAP

interface to

Quoted 1 line

an existing bridge. The user should modify this script or create a

bridge

Quoted 3 lines

some other way.

This works very well on two hosts I tried, but I'm not entirely sure

it will

Quoted 3 lines

work in all cases -- please let me know what you think.

(I remember Jason Wang mentioned that tcpdump doesn't always catch

all the

Quoted 1 line

required traffic, or that it might somehow depend on the DHCP server

-- anyone

Quoted 13 lines
Quoted 3 lines

willing to comment on that?)

The following patches have been tested with a few KVM tests (boot,

reboot,

Quoted 1 line

stress_boot, migration) with both TAP and user mode, but they could

probably

Quoted 1 line

use more testing (maybe with Python 2.6?), so if anyone is willing

to help

Quoted 5 lines

with that I'd appreciate it very much.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#27 lore raw
In reply to Michael Goldish
Quoted 25 lines · Michael Goldish

Here's my proposed TAP solution:
- The user specifies MAC ranges and may or may not specify corresponding IP ranges.
- VMs are always given MAC addresses from the user specified MAC ranges.
- MAC address ranges must be unique to each host running KVM tests -- there must be no overlap between them.
- If corresponding IP ranges are specified as well, the system will use them when trying to communicate with guests.
- If corresponding IP ranges are not specified, tcpdump will be used to listen to DHCP traffic and dynamically detect the right IP addresses. There's also a flag that can force this behavior.
- tcpdump will run in the background at all times, and collect MAC-IP pairs as soon as they are assigned by the DHCP server.  This is useful for NIC hotplugging (where IP addresses are assigned during the test itself) and generally for misbehaving guests or DHCP servers (restarting network services during the test, using very short lease times).
- It is up to the user to create the actual bridge for TAP devices; the qemu-ifup script included in one of the patches only adds the TAP interface to an existing bridge.  The user should modify this script or create a bridge some other way.

This works very well on two hosts I tried, but I'm not entirely sure it will work in all cases -- please let me know what you think.

Quoted 8 lines

(I remember Jason Wang mentioned that tcpdump doesn't always catch all the required traffic, or that it might somehow depend on the DHCP server -- anyone willing to comment on that?)

The following patches have been tested with a few KVM tests (boot, reboot, stress_boot, migration) with both TAP and user mode, but they could probably use more testing (maybe with Python 2.6?), so if anyone is willing to help with that I'd appreciate it very much.

No problem, since my development machine have python 2.6 and that's where I develop, I will test while I review your patches!