[Qemu-devel] Re: [RFC PATCH 01/14] KVM-test: Add a new macaddress pool algorithm

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

On Tue, 2010-07-20 at 09:34 +0800, Amos Kong wrote:
> Old method uses the mac address in the configuration files which could
> lead serious problem when multiple tests running in different hosts.
> 
> This patch adds a new macaddress pool algorithm, it generates the mac prefix
> based on mac address of the host which could eliminate the duplicated mac
> addresses between machines.
> 
> When user have set the mac_prefix in the configuration file, we should use it
> in stead of the dynamic generated mac prefix.

I have some additional comments besides the good points made by Michael
on his review.

> Other change:
> . Fix randomly generating mac address so that it correspond to IEEE802.
> . Update clone function to decide clone mac address or not.
> . Update get_macaddr function.
> . Add set_mac_address function.
> 
> New auto mac address pool algorithm:
> If address_index is defined, VM will get mac from config file then record mac
> in to address_pool. If address_index is not defined, VM will call
> get_mac_from_pool to auto create mac then recored mac to address_pool in
> following format:
> {'macpool': {'AE:9D:94:6A:9b:f9': ['20100310-165222-Wt7l:0']}}
> 
>   AE:9D:94:6A:9b:f9    : mac address
>   20100310-165222-Wt7l : instance attribute of VM
>   0                    : index of NIC
> 
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Feng Yang <fyang@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  0 files changed, 0 insertions(+), 0 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_utils.py b/client/tests/kvm/kvm_utils.py
> index fb2d1c2..7c0946e 100644
> --- a/client/tests/kvm/kvm_utils.py
> +++ b/client/tests/kvm/kvm_utils.py
> @@ -5,6 +5,7 @@ KVM test utility functions.
>  """
>  
>  import time, string, random, socket, os, signal, re, logging, commands, cPickle
> +import fcntl, shelve
>  from autotest_lib.client.bin import utils
>  from autotest_lib.client.common_lib import error, logging_config
>  import kvm_subprocess
> @@ -82,6 +83,104 @@ def get_sub_dict_names(dict, keyword):
>  
>  # Functions related to MAC/IP addresses
>  
> +def get_mac_from_pool(root_dir, vm, nic_index, prefix='00:11:22:33:'):
> +    """
> +    random generated mac address.

I think I got your original idea of naming this function
get_mac_from_pool (the prefix assessed is the base of the 'pool'), but
as Michael pointed out, a new MAC is being generated here and added to
the mac pool, so a more appropriate name should be used. A
straightforward suggestion would be generate_mac_address. Also, the
description of the function would be more on the lines of "Generate a
MAC address and add it to the MAC pool".

> +    1) First try to generate macaddress based on the mac address prefix.
> +    2) And then try to use total random generated mac address.
> +
> +    @param root_dir: Root dir for kvm
> +    @param vm: Here we use instance of vm
> +    @param nic_index: The index of nic.
> +    @param prefix: Prefix of mac address.
> +    @Return: Return mac address.
> +    """
> +
> +    lock_filename = os.path.join(root_dir, "mac_lock")
> +    lock_file = open(lock_filename, 'w')
> +    fcntl.lockf(lock_file.fileno() ,fcntl.LOCK_EX)
> +    mac_filename = os.path.join(root_dir, "address_pool")
> +    mac_shelve = shelve.open(mac_filename, writeback=False)

^ Indeed we should keep the mac pool under /tmp as Michael already
pointed out.

> +    mac_pool = mac_shelve.get("macpool")
> +
> +    if not mac_pool:
> +        mac_pool = {}
> +    found = False
> +
> +    val = "%s:%s" % (vm, nic_index)
> +    for key in mac_pool.keys():
> +        if val in mac_pool[key]:
> +            mac_pool[key].append(val)
> +            found = True
> +            mac = key
> +
> +    while not found:
> +        postfix = "%02x:%02x" % (random.randint(0x00,0xfe),
> +                                random.randint(0x00,0xfe))
> +        mac = prefix + postfix

^ It's more natural to use 'sufix' instead of 'postfix' here.

> +        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 not in mac_pool.keys() or 0 == len(mac_pool[mac]):
> +            mac_pool[mac] = ["%s:%s" % (vm,nic_index)]
> +            found = True
> +    mac_shelve["macpool"] = mac_pool
> +    logging.debug("generating mac addr %s " % mac)

^ Here the MAC was already generated, so here you could use something
like "Generated MAC address %s"

> +    mac_shelve.close()
> +    fcntl.lockf(lock_file.fileno(), fcntl.LOCK_UN)
> +    lock_file.close()
> +    return mac
> +
> +
> +def put_mac_to_pool(root_dir, mac, vm):
> +    """
> +    Put the macaddress back to address pool
> +
> +    @param root_dir: Root dir for kvm
> +    @param vm: Here we use instance attribute of vm

^ @param vm: Instance attribute of a vm. Also, we might want to rename
this parameter to vm_instance, to make things more clear and avoid
confusions (vm is referred in several places as a vm instance).

> +    @param mac: mac address will be put.
> +    @Return: mac address.
> +    """
> +
> +    lock_filename = os.path.join(root_dir, "mac_lock")
> +    lock_file = open(lock_filename, 'w')
> +    fcntl.lockf(lock_file.fileno() ,fcntl.LOCK_EX)
> +    mac_filename = os.path.join(root_dir, "address_pool")
> +    mac_shelve = shelve.open(mac_filename, writeback=False)
> +
> +    mac_pool = mac_shelve.get("macpool")
> +
> +    if not mac_pool or (not mac in mac_pool):
> +        logging.debug("Try to free a macaddress does no in pool")

^ logging.debug("MAC not present in the MAC pool, not modifying pool")

> +        logging.debug("macaddress is %s" % mac)
> +        logging.debug("pool is %s" % mac_pool)
> +    else:
> +        if len(mac_pool[mac]) <= 1:
> +            mac_pool.pop(mac)
> +        else:
> +            for value in mac_pool[mac]:
> +                if vm in value:
> +                    mac_pool[mac].remove(value)
> +                    break
> +            if len(mac_pool[mac]) == 0:
> +                mac_pool.pop(mac)
> +
> +    mac_shelve["macpool"] = mac_pool
> +    logging.debug("freeing mac addr %s " % mac)

^ logging.debug("Freeing mac addr %s " % mac). Also, this should go to
the beginning of the function, before the "MAC not present in the..."
message.

> +
> +    mac_shelve.close()
> +    fcntl.lockf(lock_file.fileno(), fcntl.LOCK_UN)
> +    lock_file.close()
> +    return mac
> +
> +
>  def mac_str_to_int(addr):
>      """
>      Convert MAC address string to integer.
> diff --git a/client/tests/kvm/kvm_vm.py b/client/tests/kvm/kvm_vm.py
> index 6cd0688..a9ba6e7 100755
> --- a/client/tests/kvm/kvm_vm.py
> +++ b/client/tests/kvm/kvm_vm.py
> @@ -5,7 +5,7 @@ Utility classes and functions to handle Virtual Machine creation using qemu.
>  @copyright: 2008-2009 Red Hat Inc.
>  """
>  
> -import time, socket, os, logging, fcntl, re, commands, glob
> +import time, socket, os, logging, fcntl, re, commands, shelve, glob
>  import kvm_utils, kvm_subprocess, kvm_monitor, rss_file_transfer
>  from autotest_lib.client.common_lib import error
>  from autotest_lib.client.bin import utils
> @@ -117,6 +117,7 @@ class VM:
>          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
> @@ -126,8 +127,15 @@ class VM:
>              if not glob.glob("/tmp/*%s" % self.instance):
>                  break
>  
> +        if self.mac_prefix is None:
> +            s, o = commands.getstatusoutput("ifconfig eth0")
> +            if s == 0:
> +                mac = re.findall("HWaddr (\S*)", o)[0]
> +                self.mac_prefix = '00' + mac[8:] + ':'
> +
>  
> -    def clone(self, name=None, params=None, root_dir=None, address_cache=None):
> +    def clone(self, name=None, params=None, root_dir=None,
> +                    address_cache=None, mac_clone=True):
>          """
>          Return a clone of the VM object with optionally modified parameters.
>          The clone is initially not alive and needs to be started using create().
> @@ -138,6 +146,7 @@ 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 mac_clone: Clone mac address or not.
>          """

Here if you redesign the patch as suggested by Michael, making the
unique key the string 'instance' + 'nic_index', you could add a
parameter like 'preserve_mac', to preserve the original mac address, for
things like migration.

In such cases, first the entry of the nics present on the source vm
would be removed from the MAC pool, then the new entries would be added.

>          if name is None:
>              name = self.name
> @@ -147,9 +156,19 @@ class VM:
>              root_dir = self.root_dir
>          if address_cache is None:
>              address_cache = self.address_cache
> -        return VM(name, params, root_dir, address_cache)
> +        vm = VM(name, params, root_dir, address_cache)
> +        if mac_clone:
> +            # Clone mac address by coping 'self.instance' to the new vm.
> +            vm.instance = self.instance
> +        return vm
>  
> 
> +    def free_mac_address(self):
> +        nic_num = len(kvm_utils.get_sub_dict_names(self.params, "nics"))
> +        for nic in range(nic_num):
> +            mac = self.get_macaddr(nic_index=nic)
> +            kvm_utils.put_mac_to_pool(self.root_dir, mac, vm=self.instance)

^ Here we are removing an association between a 'instance'+'nic_index'
and a MAC, so maybe this should be called 'clear_macs_from_nics'

>      def make_qemu_command(self, name=None, params=None, root_dir=None):
>          """
>          Generate a qemu command line. All parameters are optional. If a
> @@ -383,6 +402,13 @@ class VM:
>              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.get_mac_from_pool(self.root_dir,
> +                                                  vm=self.instance,
> +                                                  nic_index=vlan,
> +                                                  prefix=self.mac_prefix)
> +
>              qemu_cmd += add_nic(help, vlan, nic_params.get("nic_model"), mac,
>                                  self.netdev_id[vlan])
>              # Handle the '-net tap' or '-net user' part
> @@ -396,7 +422,7 @@ class VM:
>              if tftp:
>                  tftp = kvm_utils.get_path(root_dir, tftp)
>              qemu_cmd += add_net(help, vlan, nic_params.get("nic_mode", "user"),
> -                                nic_params.get("nic_ifname"),
> +                                self.get_ifname(vlan),
>                                  script, downscript, tftp,
>                                  nic_params.get("bootp"), redirs,
>                                  self.netdev_id[vlan])
> @@ -720,7 +746,7 @@ class VM:
>              lockfile.close()
>  
> 
> -    def destroy(self, gracefully=True):
> +    def destroy(self, gracefully=True, free_macaddr=True):
>          """
>          Destroy the VM.
>  
> @@ -731,6 +757,7 @@ class VM:
>          @param gracefully: Whether an attempt will be made to end the VM
>                  using a shell command before trying to end the qemu process
>                  with a 'quit' or a kill signal.
> +        @param free_macaddr: Whether free macaddresses when destory a vm.

^ Why would we want to conditionally remove the associations NIC-MAC?
Don't we want to do it on all cases?

>          """
>          try:
>              # Is it already dead?
> @@ -751,11 +778,18 @@ class VM:
>                          logging.debug("Shutdown command sent; waiting for VM "
>                                        "to go down...")
>                          if kvm_utils.wait_for(self.is_dead, 60, 1, 1):
> -                            logging.debug("VM is down")
> +                            logging.debug("VM is down, free mac address.")
> +                            # free mac address
> +                            if free_macaddr:
> +                                self.free_mac_address()
>                              return
>                      finally:
>                          session.close()
>  
> +            # free mac address
> +            if free_macaddr:
> +                self.free_mac_address()
> +
>              if self.monitor:
>                  # Try to destroy with a monitor command
>                  logging.debug("Trying to kill VM with monitor command...")
> @@ -881,10 +915,14 @@ class VM:
>          nic_name = nics[index]
>          nic_params = kvm_utils.get_sub_dict(self.params, nic_name)
>          if nic_params.get("nic_mode") == "tap":
> -            mac, ip = kvm_utils.get_mac_ip_pair_from_dict(nic_params)
> +            mac = self.get_macaddr(index)
>              if not mac:
>                  logging.debug("MAC address unavailable")
>                  return None
> +            else:
> +                mac = mac.lower()
> +            ip = None
> +
>              if not ip or nic_params.get("always_use_tcpdump") == "yes":
>                  # Get the IP address from the cache
>                  ip = self.address_cache.get(mac)
> @@ -897,6 +935,7 @@ class VM:
>                               for nic in nics]
>                  macs = [kvm_utils.get_mac_ip_pair_from_dict(dict)[0]
>                          for dict in nic_dicts]
> +                macs.append(mac)
>                  if not kvm_utils.verify_ip_address_ownership(ip, macs):
>                      logging.debug("Could not verify MAC-IP address mapping: "
>                                    "%s ---> %s" % (mac, ip))
> @@ -925,6 +964,71 @@ class VM:
>                               "redirected" % port)
>              return self.redirs.get(port)
>  
> +    def get_ifname(self, nic_index=0):
> +        """
> +        Return the ifname of tap device for the guest nic.
> +
> +        @param nic_index: Index of the NIC
> +        """
> +
> +        nics = kvm_utils.get_sub_dict_names(self.params, "nics")
> +        nic_name = nics[nic_index]
> +        nic_params = kvm_utils.get_sub_dict(self.params, nic_name)
> +        if nic_params.get("nic_ifname"):
> +            return nic_params.get("nic_ifname")
> +        else:
> +            return "%s_%s_%s" % (nic_params.get("nic_model"),
> +                                 nic_index, self.vnc_port)
> +
> +    def get_macaddr(self, nic_index=0):
> +        """
> +        Return the macaddr of guest nic.
> +
> +        @param nic_index: Index of the NIC
> +        """
> +        mac_filename = os.path.join(self.root_dir, "address_pool")
> +        mac_shelve = shelve.open(mac_filename, writeback=False)
> +        mac_pool = mac_shelve.get("macpool")
> +        val = "%s:%s" % (self.instance, nic_index)
> +        for key in mac_pool.keys():
> +            if val in mac_pool[key]:
> +                return key
> +        return None
> +
> +    def set_mac_address(self, mac, nic_index=0, shareable=False):
> +        """
> +        Set mac address for guest. Note: It just update address pool.
> +
> +        @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_filename = os.path.join(self.root_dir, "mac_lock")
> +        lock_file = open(lock_filename, 'w')
> +        fcntl.lockf(lock_file.fileno() ,fcntl.LOCK_EX)
> +        mac_filename = os.path.join(self.root_dir, "address_pool")
> +        mac_shelve = shelve.open(mac_filename, writeback=False)
> +        mac_pool = mac_shelve.get("macpool")
> +        if not mac_pool:
> +            mac_pool = {}
> +        value = "%s:%s" % (self.instance, nic_index)
> +        if mac not in mac_pool.keys():
> +            for key in mac_pool.keys():
> +                if value in mac_pool[key]:
> +                    mac_pool[key].remove(value)
> +                if len(mac_pool[key]) == 0:
> +                    mac_pool.pop(key)
> +            mac_pool[mac] = [value]
> +        else:
> +            if shareable:
> +                mac_pool[mac].append(value)
> +            else:
> +                logging.error("Mac address already be used!")
> +                return False
> +        mac_shelve["macpool"] = mac_pool
> +        mac_shelve.close()
> +        fcntl.lockf(lock_file.fileno(), fcntl.LOCK_UN)
> +        lock_file.close()
>  
>      def get_pid(self):
>          """
> diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
> index fd9e72f..6710c00 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -51,7 +51,7 @@ guest_port_remote_shell = 22
>  nic_mode = user
>  #nic_mode = tap
>  nic_script = scripts/qemu-ifup
> -address_index = 0
> +#address_index = 0
>  run_tcpdump = yes
>  
>  # Misc
>