----- "Amos Kong" <ak...@redhat.com> wrote:
> From: "Amos Kong" <ak...@redhat.com>
> To: "Michael Goldish" <mgold...@redhat.com>
> Cc: autot...@test.kernel.org, qemu-devel@nongnu.org, k...@vger.kernel.org
> 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 <jasow...@redhat.com>
> > > Signed-off-by: Feng Yang <fy...@redhat.com>
> > > Signed-off-by: Amos Kong <ak...@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:'):
> >
> > 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> _______________________________________________
> Autotest mailing list
> autot...@test.kernel.org
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
