[KVM-AUTOTEST PATCH 1/7] KVM test: simplify MAC address management

* [KVM-AUTOTEST PATCH 1/7] KVM test: simplify MAC address management
@ 2010-10-24 11:01 Michael Goldish
  2010-10-24 11:01 ` [KVM-AUTOTEST PATCH 2/7] KVM test: VM.get_address(): fix handling of multiple NICs Michael Goldish
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Goldish @ 2010-10-24 11:01 UTC (permalink / raw)
  To: autotest, kvm

- Remove the VM.mac_prefix attribute
- Remove the 'root_dir' parameter from generate_mac_address() and
  free_mac_address()
- Remove the 'prefix' parameter from generate_mac_address()
- Remove the explicit setting and clearing of bits in the most significant
  byte (it is fixed to 0x9A anyway)
- Remove the 'shareable' parameter from set_mac_address() (no longer required)
- Remove the 'preserve_mac' parameter from VM.clone() and replace it with
  'mac_source' in VM.create()
- Remove overly verbose debug messages from free_mac_address()
- Replace VM.free_mac_addresses() with VM.free_mac_address()
- Use _open_mac_pool() and _close_mac_pool() to save code
- Merge _generate_mac_address_prefix() with generate_mac_address_prefix()
- Concentrate all functions that access the MAC address pool in the same module
  (kvm_utils.py)
- Minor style changes

Signed-off-by: Michael Goldish <mgoldish@redhat.com>
---
 client/tests/kvm/kvm_test_utils.py   |    2 +-
 client/tests/kvm/kvm_utils.py        |  185 +++++++++++++++++----------------
 client/tests/kvm/kvm_vm.py           |   93 ++++++------------
 client/tests/kvm/tests/mac_change.py |   11 +--
 4 files changed, 130 insertions(+), 161 deletions(-)

diff --git a/client/tests/kvm/kvm_test_utils.py b/client/tests/kvm/kvm_test_utils.py
index 585e194..d9c5a6e 100644
--- a/client/tests/kvm/kvm_test_utils.py
+++ b/client/tests/kvm/kvm_test_utils.py
@@ -187,7 +187,7 @@ def migrate(vm, env=None, mig_timeout=3600, mig_protocol="tcp",
 
     # Clone the source VM and ask the clone to wait for incoming migration
     dest_vm = vm.clone()
-    if not dest_vm.create(extra_params=mig_extra_params):
+    if not dest_vm.create(extra_params=mig_extra_params, mac_source=vm):
         raise error.TestError("Could not create dest VM")
 
     try:
diff --git a/client/tests/kvm/kvm_utils.py b/client/tests/kvm/kvm_utils.py
index a2b0a3f..778637d 100644
--- a/client/tests/kvm/kvm_utils.py
+++ b/client/tests/kvm/kvm_utils.py
@@ -11,6 +11,17 @@ from autotest_lib.client.common_lib import error, logging_config
 import kvm_subprocess
 
 
+def _lock_file(filename):
+    f = open(filename, "w")
+    fcntl.lockf(f, fcntl.LOCK_EX)
+    return f
+
+
+def _unlock_file(f):
+    fcntl.lockf(f, fcntl.LOCK_UN)
+    f.close()
+
+
 def dump_env(obj, filename):
     """
     Dump KVM test environment to a file.
@@ -83,119 +94,113 @@ def get_sub_dict_names(dict, keyword):
 
 # Functions related to MAC/IP addresses
 
-def _generate_mac_address_prefix():
-    """
-    Generate a MAC address prefix. By convention we will set KVM autotest
-    MAC addresses to start with 0x9a.
-    """
-    r = random.SystemRandom()
-    l = [0x9a, r.randint(0x00, 0x7f), r.randint(0x00, 0x7f),
-         r.randint(0x00, 0xff)]
-    prefix = ':'.join(map(lambda x: "%02x" % x, l)) + ":"
-    return prefix
+def _open_mac_pool(lock_mode):
+    lock_file = open("/tmp/mac_lock", "w+")
+    fcntl.lockf(lock_file, lock_mode)
+    pool = shelve.open("/tmp/address_pool")
+    return pool, lock_file
 
 
-def generate_mac_address_prefix():
+def _close_mac_pool(pool, lock_file):
+    pool.close()
+    fcntl.lockf(lock_file, fcntl.LOCK_UN)
+    lock_file.close()
+
+
+def _generate_mac_address_prefix(mac_pool):
     """
     Generate a random MAC address prefix and add it to the MAC pool dictionary.
     If there's a MAC prefix there already, do not update the MAC pool and just
-    return what's in there.
-    """
-    lock_file = open("/tmp/mac_lock", 'w')
-    fcntl.lockf(lock_file.fileno() ,fcntl.LOCK_EX)
-    mac_pool = shelve.open("/tmp/address_pool", writeback=False)
+    return what's in there. By convention we will set KVM autotest MAC
+    addresses to start with 0x9a.
 
-    if mac_pool.get('prefix'):
-        prefix = mac_pool.get('prefix')
-        logging.debug('Retrieved previously generated MAC prefix for this '
-                      'host: %s', prefix)
+    @param mac_pool: The MAC address pool object.
+    @return: The MAC address prefix.
+    """
+    if "prefix" in mac_pool:
+        prefix = mac_pool["prefix"]
+        logging.debug("Used previously generated MAC address prefix for this "
+                      "host: %s", prefix)
     else:
-        prefix = _generate_mac_address_prefix()
-        mac_pool['prefix'] = prefix
-        logging.debug('Generated MAC address prefix for this host: %s', prefix)
-
-    mac_pool.close()
-    fcntl.lockf(lock_file.fileno(), fcntl.LOCK_UN)
-    lock_file.close()
-
+        r = random.SystemRandom()
+        prefix = "9a:%02x:%02x:%02x:" % (r.randint(0x00, 0xff),
+                                         r.randint(0x00, 0xff),
+                                         r.randint(0x00, 0xff))
+        mac_pool["prefix"] = prefix
+        logging.debug("Generated MAC address prefix for this host: %s", prefix)
     return prefix
 
 
-def generate_mac_address(root_dir, instance_vm, nic_index, prefix=None):
+def generate_mac_address(vm_instance, nic_index):
     """
-    Random generate a MAC address and add it to the MAC pool.
+    Randomly generate a MAC address and add it to the MAC address pool.
 
-    Try to generate macaddress based on the mac address prefix, add it to a
-    dictionary 'address_pool'.
-    key = VM instance + nic index, value = mac address
-    {['20100310-165222-Wt7l:0'] : 'AE:9D:94:6A:9b:f9'}
+    Try to generate a MAC address based on a randomly generated MAC address
+    prefix and add it to a persistent dictionary.
+    key = VM instance + NIC index, value = MAC address
+    e.g. {'20100310-165222-Wt7l:0': '9a:5d:94:6a:9b:f9'}
 
-    @param root_dir: Root dir for kvm.
-    @param instance_vm: Here we use instance of vm.
-    @param nic_index: The index of nic.
-    @param prefix: Prefix of MAC address.
+    @param vm_instance: The instance attribute of a VM.
+    @param nic_index: The index of the NIC.
     @return: MAC address string.
     """
-    if prefix is None:
-        prefix = generate_mac_address_prefix()
-
-    r = random.SystemRandom()
-    lock_file = open("/tmp/mac_lock", 'w')
-    fcntl.lockf(lock_file.fileno() ,fcntl.LOCK_EX)
-    mac_pool = shelve.open("/tmp/address_pool", writeback=False)
-    found = False
-    key = "%s:%s" % (instance_vm, nic_index)
-
-    if mac_pool.get(key):
-        found = True
-        mac = mac_pool.get(key)
-
-    while not found:
-        suffix = "%02x:%02x" % (r.randint(0x00,0xfe),
-                                r.randint(0x00,0xfe))
-        mac = prefix + suffix
-        mac_list = mac.split(":")
-        # Clear multicast bit
-        mac_list[0] = int(mac_list[0],16) & 0xfe
-        # Set local assignment bit (IEEE802)
-        mac_list[0] = mac_list[0] | 0x02
-        mac_list[0] = "%02x" % mac_list[0]
-        mac = ":".join(mac_list)
-        if mac in [mac_pool.get(k) for k in mac_pool.keys()]:
+    mac_pool, lock_file = _open_mac_pool(fcntl.LOCK_EX)
+    key = "%s:%s" % (vm_instance, nic_index)
+    if key in mac_pool:
+        mac = mac_pool[key]
+    else:
+        prefix = _generate_mac_address_prefix(mac_pool)
+        r = random.SystemRandom()
+        while key not in mac_pool:
+            mac = prefix + "%02x:%02x" % (r.randint(0x00, 0xff),
+                                          r.randint(0x00, 0xff))
+            if mac in mac_pool.values():
                 continue
-        mac_pool[key] = mac
-        found = True
-    logging.debug("Generated MAC address for NIC %s: %s ", key, mac)
-
-    mac_pool.close()
-    fcntl.lockf(lock_file.fileno(), fcntl.LOCK_UN)
-    lock_file.close()
+            mac_pool[key] = mac
+            logging.debug("Generated MAC address for NIC %s: %s", key, mac)
+    _close_mac_pool(mac_pool, lock_file)
     return mac
 
 
-def free_mac_address(root_dir, instance_vm, nic_index):
+def free_mac_address(vm_instance, nic_index):
     """
-    Free mac address from address pool
+    Remove a MAC address from the address pool.
 
-    @param root_dir: Root dir for kvm
-    @param instance_vm: Here we use instance attribute of vm
-    @param nic_index: The index of nic
+    @param vm_instance: The instance attribute of a VM.
+    @param nic_index: The index of the NIC.
     """
-    lock_file = open("/tmp/mac_lock", 'w')
-    fcntl.lockf(lock_file.fileno() ,fcntl.LOCK_EX)
-    mac_pool = shelve.open("/tmp/address_pool", writeback=False)
-    key = "%s:%s" % (instance_vm, nic_index)
-    if not mac_pool or (not key in mac_pool.keys()):
-        logging.debug("NIC not present in the MAC pool, not modifying pool")
-        logging.debug("NIC: %s" % key)
-        logging.debug("Contents of MAC pool: %s" % mac_pool)
-    else:
-        logging.debug("Freeing MAC addr for NIC %s: %s", key, mac_pool[key])
-        mac_pool.pop(key)
+    mac_pool, lock_file = _open_mac_pool(fcntl.LOCK_EX)
+    key = "%s:%s" % (vm_instance, nic_index)
+    if key in mac_pool:
+        logging.debug("Freeing MAC address for NIC %s: %s", key, mac_pool[key])
+        del mac_pool[key]
+    _close_mac_pool(mac_pool, lock_file)
 
-    mac_pool.close()
-    fcntl.lockf(lock_file.fileno(), fcntl.LOCK_UN)
-    lock_file.close()
+
+def set_mac_address(vm_instance, nic_index, mac):
+    """
+    Set a MAC address in the pool.
+
+    @param vm_instance: The instance attribute of a VM.
+    @param nic_index: The index of the NIC.
+    """
+    mac_pool, lock_file = _open_mac_pool(fcntl.LOCK_EX)
+    mac_pool["%s:%s" % (vm_instance, nic_index)] = mac
+    _close_mac_pool(mac_pool, lock_file)
+
+
+def get_mac_address(vm_instance, nic_index):
+    """
+    Return a MAC address from the pool.
+
+    @param vm_instance: The instance attribute of a VM.
+    @param nic_index: The index of the NIC.
+    @return: MAC address string.
+    """
+    mac_pool, lock_file = _open_mac_pool(fcntl.LOCK_SH)
+    mac = mac_pool.get("%s:%s" % (vm_instance, nic_index))
+    _close_mac_pool(mac_pool, lock_file)
+    return mac
 
 
 def mac_str_to_int(addr):
diff --git a/client/tests/kvm/kvm_vm.py b/client/tests/kvm/kvm_vm.py
index 848ff63..51e7cfa 100755
--- a/client/tests/kvm/kvm_vm.py
+++ b/client/tests/kvm/kvm_vm.py
@@ -109,16 +109,15 @@ class VM:
         self.serial_console = None
         self.redirs = {}
         self.vnc_port = 5900
-        self.uuid = None
         self.monitors = []
         self.pci_assignable = None
+        self.netdev_id = []
+        self.uuid = None
 
         self.name = name
         self.params = params
         self.root_dir = root_dir
         self.address_cache = address_cache
-        self.mac_prefix = params.get('mac_prefix')
-        self.netdev_id = []
 
         # Find a unique identifier for this VM
         while True:
@@ -127,12 +126,8 @@ class VM:
             if not glob.glob("/tmp/*%s" % self.instance):
                 break
 
-        if self.mac_prefix is None:
-            self.mac_prefix = kvm_utils.generate_mac_address_prefix()
-
 
-    def clone(self, name=None, params=None, root_dir=None,
-                    address_cache=None, preserve_mac=True):
+    def clone(self, name=None, params=None, root_dir=None, address_cache=None):
         """
         Return a clone of the VM object with optionally modified parameters.
         The clone is initially not alive and needs to be started using create().
@@ -143,7 +138,6 @@ class VM:
         @param params: Optional new VM creation parameters
         @param root_dir: Optional new base directory for relative filenames
         @param address_cache: A dict that maps MAC addresses to IP addresses
-        @param preserve_mac: Clone mac address or not.
         """
         if name is None:
             name = self.name
@@ -153,20 +147,7 @@ class VM:
             root_dir = self.root_dir
         if address_cache is None:
             address_cache = self.address_cache
-        vm = VM(name, params, root_dir, address_cache)
-        if preserve_mac:
-            vlan = 0
-            for nic_name in kvm_utils.get_sub_dict_names(params, "nics"):
-                nic_params = kvm_utils.get_sub_dict(params, nic_name)
-                vm.set_mac_address(self.get_mac_address(vlan), vlan, True)
-                vlan += 1
-        return vm
-
-
-    def free_mac_addresses(self):
-        nic_num = len(kvm_utils.get_sub_dict_names(self.params, "nics"))
-        for i in range(nic_num):
-            kvm_utils.free_mac_address(self.root_dir, self.instance, i)
+        return VM(name, params, root_dir, address_cache)
 
 
     def make_qemu_command(self, name=None, params=None, root_dir=None):
@@ -423,16 +404,7 @@ class VM:
         for nic_name in kvm_utils.get_sub_dict_names(params, "nics"):
             nic_params = kvm_utils.get_sub_dict(params, nic_name)
             # Handle the '-net nic' part
-            mac = None
-            if "address_index" in nic_params:
-                mac = kvm_utils.get_mac_ip_pair_from_dict(nic_params)[0]
-                self.set_mac_address(mac=mac, nic_index=vlan)
-            else:
-                mac = kvm_utils.generate_mac_address(self.root_dir,
-                                                     self.instance,
-                                                     vlan,
-                                                     self.mac_prefix)
-
+            mac = self.get_mac_address(vlan)
             qemu_cmd += add_nic(help, vlan, nic_params.get("nic_model"), mac,
                                 self.netdev_id[vlan],
                                 nic_params.get("nic_extra_params"))
@@ -536,7 +508,8 @@ class VM:
 
 
     def create(self, name=None, params=None, root_dir=None,
-               for_migration=False, timeout=5.0, extra_params=None):
+               for_migration=False, timeout=5.0, extra_params=None,
+               mac_source=None):
         """
         Start the VM by running a qemu command.
         All parameters are optional. The following applies to all parameters
@@ -551,6 +524,8 @@ class VM:
         option
         @param extra_params: extra params for qemu command.e.g -incoming option
         Please use this parameter instead of for_migration.
+        @param mac_source: A VM object from which to copy MAC addresses. If not
+                specified, new addresses will be generated.
         """
         self.destroy()
 
@@ -625,6 +600,15 @@ class VM:
                 self.uuid = f.read().strip()
                 f.close()
 
+            # Generate or copy MAC addresses for all NICs
+            num_nics = len(kvm_utils.get_sub_dict_names(params, "nics"))
+            for vlan in range(num_nics):
+                mac = mac_source and mac_source.get_mac_address(vlan)
+                if mac:
+                    kvm_utils.set_mac_address(self.instance, vlan, mac)
+                else:
+                    kvm_utils.generate_mac_address(self.instance, vlan)
+
             # Assign a PCI assignable device
             self.pci_assignable = None
             pa_type = params.get("pci_assignable")
@@ -799,14 +783,10 @@ class VM:
                                       "to go down...")
                         if kvm_utils.wait_for(self.is_dead, 60, 1, 1):
                             logging.debug("VM is down, freeing mac address.")
-                            self.free_mac_addresses()
                             return
                     finally:
                         session.close()
 
-            # Free mac addresses
-            self.free_mac_addresses()
-
             if self.monitor:
                 # Try to destroy with a monitor command
                 logging.debug("Trying to kill VM with monitor command...")
@@ -846,6 +826,9 @@ class VM:
                     os.unlink(f)
                 except OSError:
                     pass
+            num_nics = len(kvm_utils.get_sub_dict_names(self.params, "nics"))
+            for vlan in range(num_nics):
+                self.free_mac_address(vlan)
 
 
     @property
@@ -1002,41 +985,25 @@ class VM:
 
     def get_mac_address(self, nic_index=0):
         """
-        Return the macaddr of guest nic.
+        Return the MAC address of a NIC.
 
         @param nic_index: Index of the NIC
         """
-        mac_pool = shelve.open("/tmp/address_pool", writeback=False)
-        key = "%s:%s" % (self.instance, nic_index)
-        if key in mac_pool.keys():
-            return mac_pool[key]
+        nic_name = kvm_utils.get_sub_dict_names(self.params, "nics")[nic_index]
+        nic_params = kvm_utils.get_sub_dict(self.params, nic_name)
+        if nic_params.get("address_index"):
+            return kvm_utils.get_mac_ip_pair_from_dict(nic_params)[0]
         else:
-            return None
+            return kvm_utils.get_mac_address(self.instance, nic_index)
 
 
-    def set_mac_address(self, mac, nic_index=0, shareable=False):
+    def free_mac_address(self, nic_index=0):
         """
-        Set mac address for guest. Note: It just update address pool.
+        Free a NIC's MAC address.
 
-        @param mac: address will set to guest
         @param nic_index: Index of the NIC
-        @param shareable: Where VM can share mac with other VM or not.
         """
-        lock_file = open("/tmp/mac_lock", 'w')
-        fcntl.lockf(lock_file.fileno() ,fcntl.LOCK_EX)
-        mac_pool = shelve.open("/tmp/address_pool", writeback=False)
-        key = "%s:%s" % (self.instance, nic_index)
-
-        if not mac in [mac_pool[i] for i in mac_pool.keys()]:
-            mac_pool[key] = mac
-        else:
-            if shareable:
-                mac_pool[key] = mac
-            else:
-                logging.error("MAC address %s is already in use!", mac)
-        mac_pool.close()
-        fcntl.lockf(lock_file.fileno(), fcntl.LOCK_UN)
-        lock_file.close()
+        kvm_utils.free_mac_address(self.instance, nic_index)
 
 
     def get_pid(self):
diff --git a/client/tests/kvm/tests/mac_change.py b/client/tests/kvm/tests/mac_change.py
index 010c395..c614e15 100644
--- a/client/tests/kvm/tests/mac_change.py
+++ b/client/tests/kvm/tests/mac_change.py
@@ -24,14 +24,11 @@ def run_mac_change(test, params, env):
         raise error.TestFail("Could not log into guest '%s'" % vm.name)
 
     old_mac = vm.get_mac_address(0)
-    new_different = False
-    while not new_different:
-        kvm_utils.free_mac_address(vm.root_dir, vm.instance, 0)
-        new_mac = kvm_utils.generate_mac_address(vm.root_dir,
-                                                 vm.instance,
-                                                 0, vm.mac_prefix)
+    while True:
+        vm.free_mac_address(0)
+        new_mac = kvm_utils.generate_mac_address(vm.instance, 0)
         if old_mac != new_mac:
-            new_different = True
+            break
     logging.info("The initial MAC address is %s", old_mac)
     interface = kvm_test_utils.get_linux_ifname(session, old_mac)
     # Start change MAC address
-- 
1.5.5.6

^ permalink raw reply related	[flat|nested] 9+ messages in thread