Re: [PATCH][KVM-AUTOTEST] TAP network support in kvm-autotest

* Re: [PATCH][KVM-AUTOTEST] TAP network support in kvm-autotest
       [not found] <893076616.211371242296910759.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com>
@ 2009-05-14 10:40 ` Michael Goldish
  2009-05-15  8:29   ` jason wang
  2009-06-02  7:06   ` sudhir kumar
  0 siblings, 2 replies; 4+ messages in thread
From: Michael Goldish @ 2009-05-14 10:40 UTC (permalink / raw)
  To: Jason Wang; +Cc: ulublin, kvm

Hi Jason,

We already have patches that implement similar functionality here in
TLV, as mentioned in the to-do list (item #4 under 'Framework').
They're not yet committed upstream because they're still quite fresh.

Still, your patch looks good and is quite similar to mine. The main
difference is that I use MAC/IP address pools specified by the user,
instead of random MACs with arp/nmap to detect the matching IP
addresses.

I will post my patch to the mailing list soon, but it will come
together with quite a few other patches that I haven't posted yet, so
please be patient.

Comments/questions:

Why do you use nmap in addition to arp? In what cases will arp not
suffice? I'm a little put off by the fact that nmap imposes an
additional requirement on the host. Three hosts I've tried don't come
with nmap installed by default.

Please see additional comments below.

----- "Jason Wang" <jasowang@redhat.com> wrote:

> Hi All:
> This patch tries to add tap network support in kvm-autotest. Multiple
> nics connected to different bridges could be achieved through this
> script. Public bridge is important for testing real network traffic
> and migration. The patch gives each nic with randomly generated mac
> address. The ip address required in the test could be dynamically
> probed through nmap/arp. Only the ip address of first NIC is used
> through the test.
> 
> Example:
> nics = nic1 nic2
> network = bridge
> bridge = switch
> ifup =/etc/qemu-ifup-switch
> ifdown =/etc/qemu-ifdown-switch
> 
> This would make the virtual machine have two nics both of which are
> connected to a bridge with the name of 'switch'. Ifup/ifdown scripts
> are also specified.
> 
> Another Example:
> nics = nic1 nic2
> network = bridge
> bridge = switch
> bridge_nic2 = virbr0
> ifup =/etc/qemu-ifup-switch
> ifup_nic2 = /etc/qemu-ifup-virbr0
> 
> This would makes the virtual machine have two nics: nic1 are connected
> to bridge 'switch' and nci2 are connected to bridge 'virbr0'.
> 
> Public mode and user mode nic could also be mixed:
> nics = nic1 nic2
> network = bridge
> network_nic2 = user
> 
> Looking forward for comments and suggestions.
> 
> From: jason <jasowang@redhat.com>
> Date: Wed, 13 May 2009 16:15:28 +0800
> Subject: [PATCH] Add tap networking support.
> 
> ---
>  client/tests/kvm_runtest_2/kvm_utils.py |    7 +++
>  client/tests/kvm_runtest_2/kvm_vm.py    |   74
> ++++++++++++++++++++++++++-----
>  2 files changed, 69 insertions(+), 12 deletions(-)
> 
> diff --git a/client/tests/kvm_runtest_2/kvm_utils.py
> b/client/tests/kvm_runtest_2/kvm_utils.py
> index be8ad95..0d1f7f8 100644
> --- a/client/tests/kvm_runtest_2/kvm_utils.py
> +++ b/client/tests/kvm_runtest_2/kvm_utils.py
> @@ -773,3 +773,10 @@ def md5sum_file(filename, size=None):
>          size -= len(data)
>      f.close()
>      return o.hexdigest()
> +
> +def random_mac():
> +    mac=[0x00,0x16,0x30,
> +         random.randint(0x00,0x09),
> +         random.randint(0x00,0x09),
> +         random.randint(0x00,0x09)]
> +    return ':'.join(map(lambda x: "%02x" %x,mac))

Random MAC addresses will not necessarily work everywhere, as far as
I know. That's why I prefer user specified MAC/IP address ranges.

> diff --git a/client/tests/kvm_runtest_2/kvm_vm.py
> b/client/tests/kvm_runtest_2/kvm_vm.py
> index fab839f..ea7dab6 100644
> --- a/client/tests/kvm_runtest_2/kvm_vm.py
> +++ b/client/tests/kvm_runtest_2/kvm_vm.py
> @@ -105,6 +105,10 @@ class VM:
>          self.qemu_path = qemu_path
>          self.image_dir = image_dir
>          self.iso_dir = iso_dir
> +        self.macaddr = []
> +        for nic_name in kvm_utils.get_sub_dict_names(params,"nics"):
> +            macaddr = kvm_utils.random_mac()
> +            self.macaddr.append(macaddr)
>
>      def verify_process_identity(self):
>          """Make sure .pid really points to the original qemu
> process.
> @@ -189,9 +193,25 @@ class VM:
>          for nic_name in kvm_utils.get_sub_dict_names(params,
> "nics"):
>              nic_params = kvm_utils.get_sub_dict(params, nic_name)
>              qemu_cmd += " -net nic,vlan=%d" % vlan
> +            net = nic_params.get("network")
> +            if net == "bridge":
> +                qemu_cmd += ",macaddr=%s" % self.macaddr[vlan]
>              if nic_params.get("nic_model"):
>                  qemu_cmd += ",model=%s" % nic_params.get("nic_model")
> -            qemu_cmd += " -net user,vlan=%d" % vlan
> +            if net == "bridge":
> +                qemu_cmd += " -net tap,vlan=%d" % vlan
> +                ifup = nic_params.get("ifup")
> +                if ifup:
> +                    qemu_cmd += ",script=%s" % ifup
> +                else:
> +                    qemu_cmd += ",script=/etc/qemu-ifup"

Why not just leave 'script' out if the user doesn't specify 'ifup'?
There's no good reason to prefer /etc/qemu-ifup to /etc/kvm-ifup or
anything else, so I think it's best to leave it up to qemu if the
user has no preference. It's also slightly shorter.

> +                ifdown = nic_params.get("ifdown")
> +                if ifdown:
> +                    qemu_cmd += ",downscript=%s" % ifdown
> +                else:
> +                    qemu_cmd += ",downscript=no"

The same applies here.

This is just my opinion; I'd like to hear your thoughts on this too.

> +            else:
> +                qemu_cmd += " -net user,vlan=%d" % vlan
>              vlan += 1
>  
>          mem = params.get("mem")
> @@ -206,11 +226,11 @@ class VM:
>          extra_params = params.get("extra_params")
>          if extra_params:
>              qemu_cmd += " %s" % extra_params
> -
> +            
>          for redir_name in kvm_utils.get_sub_dict_names(params,
> "redirs"):
>              redir_params = kvm_utils.get_sub_dict(params,
> redir_name)
>              guest_port = int(redir_params.get("guest_port"))
> -            host_port = self.get_port(guest_port)
> +            host_port = self.get_port(guest_port,True)
>              qemu_cmd += " -redir tcp:%s::%s" % (host_port,
> guest_port)
>  
>          if params.get("display") == "vnc":
> @@ -467,27 +487,57 @@ class VM:
>          If port redirection is used, return 'localhost' (the guest
> has no IP
>          address of its own).  Otherwise return the guest's IP
> address.
>          """
> -        # Currently redirection is always used, so return
> 'localhost'
> -        return "localhost"
> +        if self.params.get("network") == "bridge":
> +            # probing ip address through arp
> +            bridge_name = self.params['bridge']
> +            macaddr = self.macaddr[0]

I think VM.get_address() should take an index parameter, instead of
just return the first address. The index parameter can default to 0.

> +            lines = os.popen("arp -a").readlines()
> +            for line in lines:
> +                if macaddr in line:
> +                    return line.split()[1].strip('()')
> +                
> +            # probing ip address through nmap
> +            lines = os.popen("ip route").readlines()
> +            birdge_network = None
> +            for line in lines:
> +                if bridge_name in line:
> +                    bridge_network = line.split()[0]
> +                    break
> +                
> +            if bridge_network != None:
> +                lines = os.popen("nmap -sP -n %s" %
> bridge_network).readlines()
> +                lastline = None
> +                for line in lines:
> +                    if macaddr in line:
> +                        return lastline.split()[1]
> +                    lastline = line
> +
> +            # could not found ip address
> +            return None
> +        else:
> +            return "localhost"
>  
> -    def get_port(self, port):
> +    def get_port(self, port, query = False):
>          """Return the port in host space corresponding to port in
> guest space.
>  
>          If port redirection is used, return the host port redirected
> to guest port port.
>          Otherwise return port.
>          """
> -        # Currently redirection is always used, so use the redirs
> dict
> -        if self.redirs.has_key(port):
> -            return self.redirs[port]
> +
> +        if query == True or self.params.get("network") != "bridge":

Why do we need a 'query' parameter here? It looks to me like
self.params.get("network") != "bridge"
should suffice.

> +            if self.redirs.has_key(port):
> +                return self.redirs[port]
> +            else:
> +                kvm_log.debug("Warning: guest port %s requested but
> not redirected" % port)
> +                return None
>          else:
> -            kvm_log.debug("Warning: guest port %s requested but not
> redirected" % port)
> -            return None
> +            return port
>  
>      def is_sshd_running(self, timeout=10):
>          """Return True iff the guest's SSH port is responsive."""
>          address = self.get_address()
>          port = self.get_port(int(self.params.get("ssh_port")))
> -        if not port:
> +        if not port or not address:
>              return False
>          return kvm_utils.is_sshd_running(address, port,
> timeout=timeout)

Again, I will do my best to post my patches soon.

Thanks,
Michael

^ permalink raw reply	[flat|nested] 4+ messages in thread