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

[Amos Kong]

On Tue, Jul 20, 2010 at 01:19:39PM +0300, Michael Goldish wrote:
>

Michael,

Thanks for your comments. Let's simplify this method together.

> On 07/20/2010 04:34 AM, 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.
> > 
> > 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
> 
> Why do you use the mac address as a key, instead of the instance string
> + nic index?  When the mac address is used as a key, each key has a list
> of values instead of just one value.  This order seems unnatural.  If it
> were the other way around (i.e. key = VM instance + nic index, value =
> mac address), then each key would have exactly one value, and I think
> this patch would be shorter and simpler.

One mac address may be used by two VMs, eg. migration.
 
> > Signed-off-by: Jason Wang <address@hidden>
> > Signed-off-by: Feng Yang <address@hidden>
> > Signed-off-by: Amos Kong <address@hidden>
> > ---
> >  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:'):
> 
> The name of this function is confusing because it does the exact
> opposite: it puts a mac address in address_pool.  Maybe the pool you're
> referring to in the name isn't address_pool, but still a less confusing
> name should probably be used.

How about allocate_mac(...) ?
address_pool -> address_container

Allocate mac address and record into address_container.

 
> > +    """
> > +    random generated mac address.
> > +
> > +    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")
> 
> Maybe it makes sense to put address_pool and the lock file in /tmp,
> where they can be shared by more than a single autotest instance running
> on the same host (unlikely, but theoretically possible).

good idea.
 
> > +    mac_shelve = shelve.open(mac_filename, writeback=False)
> > +
> > +    mac_pool = mac_shelve.get("macpool")
> 
> Why is this 'macpool' needed?  Why not put the keys directly in the
> shelve object?
 
yes, put keys directly in the shelve object is better.

> > +    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)
> 
> Why append val to mac_pool[key] if val is already in mac_pool[key]?

need drop it.

> > +            found = True
> > +            mac = key
> > +
> > +    while not found:
> > +        postfix = "%02x:%02x" % (random.randint(0x00,0xfe),
> > +                                random.randint(0x00,0xfe))
> > +        mac = prefix + postfix
> > +        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]
> 
> Why is this needed?  Most mac addresses begin with 00. If the mac
> address is generated from the address of eth0 (using the method in this
> patch) it begins with 00, which is fine. If the prefix is set by the
> user using mac_prefix, I don't think we should modify it.


linux-2.6/include/linux/etherdevice.h

/**
 * random_ether_addr - Generate software assigned random Ethernet address
 * @addr: Pointer to a six-byte array containing the Ethernet address
 *
 * Generate a random Ethernet address (MAC) that is not multicast
 * and has the local assigned bit set.
 */
static inline void random_ether_addr(u8 *addr)
{
        get_random_bytes (addr, ETH_ALEN);
        addr [0] &= 0xfe;       /* clear multicast bit */
        addr [0] |= 0x02;       /* set local assignment bit (IEEE802) */
}

 
> > +        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)
> > +
> > +    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):
> 
> This function removes a mac address from address_pool.  I wonder why you
> chose this name.

address_pool file is a container to record macaddress.
The 'pool' we get/put mac is an abstract resource pool.
This is really confused ;)
 
> > +    """
> > +    Put the macaddress back to address pool
> > +
> > +    @param root_dir: Root dir for kvm
> > +    @param vm: Here we use instance attribute of vm
> > +    @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("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)
> > +
> > +    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.
> >          """
> >          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
> 
> Copying self.instance isn't a good idea because the monitor, serial
> console and testlog filenames use self.instance.  self.instance should
> be unique.  If we still want to use the same mac address for 2 VMs, for
> example in migration, a different solution should be found.

We almost only clone mac in migration test, the src guest would be killed.
If the instance of dest guest is same as src guest, the serial connection would 
not break,
it would continually write log to original log file.
 
> > +        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)
> > +
> >      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),
> 
> Why is get_ifname() useful here?

dynamically generate the ifname is better.
 
> >                                  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.
> >          """
> >          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)
> 
> What's the purpose of this string?

Just avoid repeated ifname.  The vnc_port is unique for each VM, nic_index is 
unique for each nic of one VM.
 
> > +    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
> > 
> > 
>