Re: [KVM-AUTOTEST PATCH 3/7] [RFC] KVM test: remove all code related to the old MAC address pool method

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

On Sun, 2010-10-24 at 13:01 +0200, Michael Goldish wrote:
> This patch removes all code related to the old manual method
> (address_pools.cfg).
> 
> Note that now running in TAP mode requires an external DHCP server that accepts
> *any* MAC address, because MAC addresses are randomly generated and cannot be
> manually configured.

Yes, this patch looks good. I don't think there are much DHCP servers
that do not accept any MAC address. It might be good to document this
information on the wiki though.

> Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> ---
>  client/tests/kvm/kvm_utils.py                      |  159 --------------------
>  client/tests/kvm/kvm_vm.py                         |   34 ++---
>  client/tests/kvm/tests/physical_resources_check.py |   11 +-
>  client/tests/kvm/tests/stress_boot.py              |    3 -
>  client/tests/kvm/tests_base.cfg.sample             |    2 -
>  5 files changed, 16 insertions(+), 193 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_utils.py b/client/tests/kvm/kvm_utils.py
> index 778637d..f749f8d 100644
> --- a/client/tests/kvm/kvm_utils.py
> +++ b/client/tests/kvm/kvm_utils.py
> @@ -203,165 +203,6 @@ def get_mac_address(vm_instance, nic_index):
>      return mac
>  
> 
> -def mac_str_to_int(addr):
> -    """
> -    Convert MAC address string to integer.
> -
> -    @param addr: String representing the MAC address.
> -    """
> -    return sum(int(s, 16) * 256 ** i
> -               for i, s in enumerate(reversed(addr.split(":"))))
> -
> -
> -def mac_int_to_str(addr):
> -    """
> -    Convert MAC address integer to string.
> -
> -    @param addr: Integer representing the MAC address.
> -    """
> -    return ":".join("%02x" % (addr >> 8 * i & 0xFF)
> -                    for i in reversed(range(6)))
> -
> -
> -def ip_str_to_int(addr):
> -    """
> -    Convert IP address string to integer.
> -
> -    @param addr: String representing the IP address.
> -    """
> -    return sum(int(s) * 256 ** i
> -               for i, s in enumerate(reversed(addr.split("."))))
> -
> -
> -def ip_int_to_str(addr):
> -    """
> -    Convert IP address integer to string.
> -
> -    @param addr: Integer representing the IP address.
> -    """
> -    return ".".join(str(addr >> 8 * i & 0xFF)
> -                    for i in reversed(range(4)))
> -
> -
> -def offset_mac(base, offset):
> -    """
> -    Add offset to a given MAC address.
> -
> -    @param base: String representing a MAC address.
> -    @param offset: Offset to add to base (integer)
> -    @return: A string representing the offset MAC address.
> -    """
> -    return mac_int_to_str(mac_str_to_int(base) + offset)
> -
> -
> -def offset_ip(base, offset):
> -    """
> -    Add offset to a given IP address.
> -
> -    @param base: String representing an IP address.
> -    @param offset: Offset to add to base (integer)
> -    @return: A string representing the offset IP address.
> -    """
> -    return ip_int_to_str(ip_str_to_int(base) + offset)
> -
> -
> -def get_mac_ip_pair_from_dict(dict):
> -    """
> -    Fetch a MAC-IP address pair from dict and return it.
> -
> -    The parameters in dict are expected to conform to a certain syntax.
> -    Typical usage may be:
> -
> -    address_ranges = r1 r2 r3
> -
> -    address_range_base_mac_r1 = 55:44:33:22:11:00
> -    address_range_base_ip_r1 = 10.0.0.0
> -    address_range_size_r1 = 16
> -
> -    address_range_base_mac_r2 = 55:44:33:22:11:40
> -    address_range_base_ip_r2 = 10.0.0.60
> -    address_range_size_r2 = 25
> -
> -    address_range_base_mac_r3 = 55:44:33:22:12:10
> -    address_range_base_ip_r3 = 10.0.1.20
> -    address_range_size_r3 = 230
> -
> -    address_index = 0
> -
> -    All parameters except address_index specify a MAC-IP address pool.  The
> -    pool consists of several MAC-IP address ranges.
> -    address_index specified the index of the desired MAC-IP pair from the pool.
> -
> -    @param dict: The dictionary from which to fetch the addresses.
> -    """
> -    index = int(dict.get("address_index", 0))
> -    for mac_range_name in get_sub_dict_names(dict, "address_ranges"):
> -        mac_range_params = get_sub_dict(dict, mac_range_name)
> -        mac_base = mac_range_params.get("address_range_base_mac")
> -        ip_base = mac_range_params.get("address_range_base_ip")
> -        size = int(mac_range_params.get("address_range_size", 1))
> -        if index < size:
> -            return (mac_base and offset_mac(mac_base, index),
> -                    ip_base and offset_ip(ip_base, index))
> -        index -= size
> -    return (None, None)
> -
> -
> -def get_sub_pool(dict, piece, num_pieces):
> -    """
> -    Split a MAC-IP pool and return a single requested piece.
> -
> -    For example, get_sub_pool(dict, 0, 3) will split the pool in 3 pieces and
> -    return a dict representing the first piece.
> -
> -    @param dict: A dict that contains pool parameters.
> -    @param piece: The index of the requested piece.  Should range from 0 to
> -        num_pieces - 1.
> -    @param num_pieces: The total number of pieces.
> -    @return: A copy of dict, modified to describe the requested sub-pool.
> -    """
> -    range_dicts = [get_sub_dict(dict, name) for name in
> -                   get_sub_dict_names(dict, "address_ranges")]
> -    if not range_dicts:
> -        return dict
> -    ranges = [[d.get("address_range_base_mac"),
> -               d.get("address_range_base_ip"),
> -               int(d.get("address_range_size", 1))] for d in range_dicts]
> -    total_size = sum(r[2] for r in ranges)
> -    base = total_size * piece / num_pieces
> -    size = total_size * (piece + 1) / num_pieces - base
> -
> -    # Find base of current sub-pool
> -    for i in range(len(ranges)):
> -        r = ranges[i]
> -        if base < r[2]:
> -            r[0] = r[0] and offset_mac(r[0], base)
> -            r[1] = r[1] and offset_ip(r[1], base)
> -            r[2] -= base
> -            break
> -        base -= r[2]
> -
> -    # Collect ranges up to end of current sub-pool
> -    new_ranges = []
> -    for i in range(i, len(ranges)):
> -        r = ranges[i]
> -        new_ranges.append(r)
> -        if size <= r[2]:
> -            r[2] = size
> -            break
> -        size -= r[2]
> -
> -    # Write new dict
> -    new_dict = dict.copy()
> -    new_dict["address_ranges"] = " ".join("r%d" % i for i in
> -                                          range(len(new_ranges)))
> -    for i in range(len(new_ranges)):
> -        new_dict["address_range_base_mac_r%d" % i] = new_ranges[i][0]
> -        new_dict["address_range_base_ip_r%d" % i] = new_ranges[i][1]
> -        new_dict["address_range_size_r%d" % i] = new_ranges[i][2]
> -    return new_dict
> -
> -
>  def verify_ip_address_ownership(ip, macs, timeout=10.0):
>      """
>      Use arping and the ARP cache to make sure a given IP address belongs to one
> diff --git a/client/tests/kvm/kvm_vm.py b/client/tests/kvm/kvm_vm.py
> index f3e803b..2db916f 100755
> --- a/client/tests/kvm/kvm_vm.py
> +++ b/client/tests/kvm/kvm_vm.py
> @@ -920,21 +920,18 @@ class VM:
>                  logging.debug("MAC address unavailable")
>                  return None
>              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)
> -                if not ip:
> -                    logging.debug("Could not find IP address for MAC address: "
> -                                  "%s" % mac)
> -                    return None
> -                # Make sure the IP address is assigned to this guest
> -                macs = [self.get_mac_address(i) for i in range(len(nics))]
> -                if not kvm_utils.verify_ip_address_ownership(ip, macs):
> -                    logging.debug("Could not verify MAC-IP address mapping: "
> -                                  "%s ---> %s" % (mac, ip))
> -                    return None
> +            # Get the IP address from the cache
> +            ip = self.address_cache.get(mac)
> +            if not ip:
> +                logging.debug("Could not find IP address for MAC address: %s" %
> +                              mac)
> +                return None
> +            # Make sure the IP address is assigned to this guest
> +            macs = [self.get_mac_address(i) for i in range(len(nics))]
> +            if not kvm_utils.verify_ip_address_ownership(ip, macs):
> +                logging.debug("Could not verify MAC-IP address mapping: "
> +                              "%s ---> %s" % (mac, ip))
> +                return None
>              return ip
>          else:
>              return "localhost"
> @@ -985,12 +982,7 @@ class VM:
>  
>          @param nic_index: Index of the NIC
>          """
> -        nic_name = kvm_utils.get_sub_dict_names(self.params, "nics")[nic_index]
> -        nic_params = kvm_utils.get_sub_dict(self.params, nic_name)
> -        if nic_params.get("address_index"):
> -            return kvm_utils.get_mac_ip_pair_from_dict(nic_params)[0]
> -        else:
> -            return kvm_utils.get_mac_address(self.instance, nic_index)
> +        return kvm_utils.get_mac_address(self.instance, nic_index)
>  
> 
>      def free_mac_address(self, nic_index=0):
> diff --git a/client/tests/kvm/tests/physical_resources_check.py b/client/tests/kvm/tests/physical_resources_check.py
> index 6c8e154..682c7b2 100644
> --- a/client/tests/kvm/tests/physical_resources_check.py
> +++ b/client/tests/kvm/tests/physical_resources_check.py
> @@ -123,14 +123,9 @@ def run_physical_resources_check(test, params, env):
>      found_mac_addresses = re.findall("macaddr=(\S+)", o)
>      logging.debug("Found MAC adresses: %s" % found_mac_addresses)
>  
> -    nic_index = 0
> -    for nic_name in kvm_utils.get_sub_dict_names(params, "nics"):
> -        nic_params = kvm_utils.get_sub_dict(params, nic_name)
> -        if "address_index" in nic_params:
> -            mac, ip = kvm_utils.get_mac_ip_pair_from_dict(nic_params)
> -        else:
> -            mac = vm.get_mac_address(nic_index)
> -            nic_index += 1
> +    num_nics = len(kvm_utils.get_sub_dict_names(params, "nics"))
> +    for nic_index in range(num_nics):
> +        mac = vm.get_mac_address(nic_index)
>          if not string.lower(mac) in found_mac_addresses:
>              n_fail += 1
>              logging.error("MAC address mismatch:")
> diff --git a/client/tests/kvm/tests/stress_boot.py b/client/tests/kvm/tests/stress_boot.py
> index 0d3ed07..b7916b4 100644
> --- a/client/tests/kvm/tests/stress_boot.py
> +++ b/client/tests/kvm/tests/stress_boot.py
> @@ -28,7 +28,6 @@ def run_stress_boot(tests, params, env):
>  
>      num = 2
>      sessions = [session]
> -    address_index = int(params.get("clone_address_index_base", 10))
>  
>      # boot the VMs
>      while num <= int(params.get("max_vms")):
> @@ -36,7 +35,6 @@ def run_stress_boot(tests, params, env):
>              # clone vm according to the first one
>              vm_name = "vm" + str(num)
>              vm_params = vm.get_params().copy()
> -            vm_params["address_index"] = str(address_index)
>              curr_vm = vm.clone(vm_name, vm_params)
>              kvm_utils.env_register_vm(env, vm_name, curr_vm)
>              logging.info("Booting guest #%d" % num)
> @@ -56,7 +54,6 @@ def run_stress_boot(tests, params, env):
>                  if se.get_command_status(params.get("alive_test_cmd")) != 0:
>                      raise error.TestFail("Session #%d is not responsive" % i)
>              num += 1
> -            address_index += 1
>  
>          except (error.TestFail, OSError):
>              for se in sessions:
> diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
> index 769d750..5bca544 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -54,7 +54,6 @@ guest_port_remote_shell = 22
>  nic_mode = user
>  #nic_mode = tap
>  nic_script = scripts/qemu-ifup
> -#address_index = 0
>  run_tcpdump = yes
>  
>  # Misc
> @@ -274,7 +273,6 @@ variants:
>          type = stress_boot
>          max_vms = 5    
>          alive_test_cmd = uname -a
> -        clone_address_index_base = 10
>          login_timeout = 240
>          kill_vm = yes
>          kill_vm_vm1 = no