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

[Michael Goldish]

On 07/20/2010 04:44 PM, Amos Kong wrote:
> 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.

Sure, that's why I thought the opposite direction would be better: keys
= VMs (nics), values = mac addresses.  That way we have one value per
key, instead of a list of values per key.

To clarify, instead of using:

{'AE:9D:94:6A:9b:f9': ['20100310-165222-Wt7l:0',
'20100310-165222-Wt7l:1', '20100310-165222-Wt7l:2']}

I suggest:

{'20100310-165222-Wt7l:0': 'AE:9D:94:6A:9b:f9',
 '20100310-165222-Wt7l:1': 'AE:9D:94:6A:9b:f9',
 '20100310-165222-Wt7l:2': 'AE:9D:94:6A:9b:f9'}

>>> 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.

Yes, something like that, sounds less confusing.

>>> +    """
>>> +    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) */
> }

I saw this googling, but I still don't understand why it's necessary
when real devices start with 00.

>>> +        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 ;)

Yes, I guessed it would be something like that.  Still a bit confusing,
I agree, but it's not a big deal to change function names.

>>> +    """
>>> +    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.

What about the monitor(s)?  The fact that the serial console connection
doesn't break doesn't necessarily mean everything's OK.

>>> +        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.

But why?  Is this ifname useful for any test?

>>>                                  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.

self.instance should also be unique, though it's quite long.

>>> +    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
>>>
>>>
>>
>