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

[Feng Yang]

----- "Amos Kong" <address@hidden> wrote:

> From: "Amos Kong" <address@hidden>
> To: "Michael Goldish" <address@hidden>
> Cc: address@hidden, address@hidden, address@hidden
> Sent: Tuesday, July 20, 2010 9:44:38 PM GMT +08:00 Beijing / Chongqing / Hong 
> Kong / Urumqi
> Subject: Re: [Autotest] [Qemu-devel] [RFC PATCH 01/14] KVM-test: Add a new 
> macaddress pool    algorithm
>
> 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.

We can not simply drop the code.  
Here append val to mac_pool[key] just for handle the issue during migration.

When kill the resource VM, corresponding mac will be freed. If we do not append 
val to mac_pool[key] here, then there is not mac record for dest VM.

We should find a better way to handler this issue.


> 
> > > +            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
> > > 
> > > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> _______________________________________________
> Autotest mailing list
> address@hidden
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest