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 <jasowang@xxxxxxxxxx>
>>> Signed-off-by: Feng Yang <fyang@xxxxxxxxxx>
>>> Signed-off-by: Amos Kong <akong@xxxxxxxxxx>
>>> ---
>>> 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
>>>
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
